JIT: workaround for unreliable change detection in fgReorderBlocks

[AndyAyersMS]

This method costs trees, which can in turn modify the IR by swapping operands.
As a result the bool value returned doesn't properly reflect whether any
changes happened.

This impacts proper reporting phase status by `optOptimizeLayout'. Since phase
status just gates post-phase dumps and checks, we'll just claim this phase
always modifies IR.

Fixes #48495.

[AndyAyersMS]

cc @dotnet/jit-contrib

[AndyAyersMS]

Looks like the preceding phase (invert loops) may have the same problem, given #48494.

So will look to extend this fix to cover that case too.

[AndyAyersMS]

Kind of on the fence here. On the one had it would be nice to be able to cost trees without side effecting them; on the other hand an accurate costing (at least as accurate as it is ever going to be) more or less entails opportunistic swapping (and address mode marking).

So three options to pursue... I have chosen the simplest for now:

• assume these phases always change IR (the only downside is extra dumps / checks in non-release)
• update costing APIs to allow a side-effect free mode
• update costing APIs to return whether they caused any side effects (other than setting costs)

[AndyAyersMS]

I suppose there is a fourth option:

• use the logic the current post-phase debug check as a way of detecting that a phase made changes. In particular the check that is firing here is that some new tree node was created.

I'm more in favor of having phases be deliberate in what they change, so decided not to pursue this one.

None of this is crucial for now, but at some point we may want to report finer-grained phase status and relying on to guide release behaviors.

[BruceForstall]

I somewhat prefer the 3rd:

update costing APIs to return whether they caused any side effects (other than setting costs)
as long as it can also be checked somehow. If it's expensive, though, then going "conservative" and assuming changes doesn't seem too bad an alternative.

[briansull]

You could also add an additional state MODIFIED_TREE here instead of using MODIFIED_EVERYTHING

[AndyAyersMS]

Right -- I originally had finer-grained status in mind, but wasn't quite sure what the right breakout should be, so just have everything/nothing for now.

[briansull]

Looks OK

[AndyAyersMS]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[trylek]

Great, thank you! It would be great if you could run this change through the shiproom and push it to Preview 2 that's just forked.

[AndyAyersMS]

It would be great if you could run this change through the shiproom

Sure.

[AndyAyersMS]

I'm fairly sure the outerloop failures are unrelated to this change. But I'm reluctant to merge with things looking this red.

Lots of instances of tracing\\eventpipe\\eventsvalidation\\ThreadPool\\ThreadPool failing, likely #48543.

And some pal test failures on Linux, dups of #48496.

And a jit-format failure on Linux, looks like jit-format is crashing; the patch artifact is a zero sized file.

[AndyAyersMS]

Local run of jit-format on linux was clean.

[AndyAyersMS]

Looked again at the failures and don't see anything related. So am going to merge after all.