In D94768#2504656, @sebastian-ne wrote:

Isn't the point to stop using these meaningless names?

I don’t know, I just tried to stop and resume compilation with llc and it failed at these tokens.
What would the better way be?

LGTM.

"HasModifiers should not be true if at least one modifier is used."

LGTM, thanks!

Rebase.

In D87464#2476205, @xbolva00 wrote:

Reping

Rebase. Pass Op0/1DemandedBits into SimplifyMultipleUseDemandedBits.

Rebase.

Rebase. D87236 seems to have fixed the code quality regressions.

This is very similar to D87145.

Thanks!

Thanks for doing the timings. The patch seems reasonable to me. I'll approve it but it would be great if you could find at least one other reviewer to take a look too.

LGTM. Any reason not to do this for SMAX and SMIN too?

In rG23b41986527a3fc5615480a8f7a0b0debd5fcef4#969903, @RKSimon wrote:

@foad Can you raise a patch please?

This could do with an exhaustive unit test.

I think improving the legacy pass manager is fine (after all it is taking a very long time for the new pass manager to supersede it). But I think any patch that claims to do less work overall, or speed something up, needs *some* kind of evidence that it actually has the desired effect.

Does this make any difference in practice? E.g. does the output of opt -O1 -debug-pass=Executions change, or can you measure any timing difference?

LGTM with comment fix.

Looks OK to me. Does this enable any immediate simplifications? Or is it just to help with future work?

In D94406#2490053, @sebastian-ne wrote:

I think the intention was that getMemOperandsWithOffsetWidth should return false if no base operands were found (perhaps MachineScheduler could assert this?)

I’d argue that in this case, we found the base operands and we know that except for the constant offset, they are zero.
Also, we want these operations to be claused, I think this is the likely case for stack accesses in entry-points.

Maybe reword the description to make it clear that it only affects the names of pseudos, not real instructions, assuming I got that right?

I think the intention was that getMemOperandsWithOffsetWidth should return false if no base operands were found (perhaps MachineScheduler could assert this?): https://reviews.llvm.org/source/llvm-github/browse/main/llvm/include/llvm/CodeGen/TargetInstrInfo.h$1304

In D86618#2484439, @arsenm wrote:

Is this still needed?

LGTM.

This is intended to supersede D91066 and D91064.

Patch looks fine to me. Why is it useful to terminate on scc0 instead of scc1, or is it an arbitrary choice? Could you give a slightly more realistic example of how it would be used? Your tests all have:

S_CMP_EQ_U32 killed $sgpr0, 0, implicit-def $scc
SI_EARLY_TERMINATE_SCC0 implicit $scc, implicit $exec

which will terminate early if sgpr0 is not 0. Is the idea that this would be used when we're just about to AND sgpr0 into exec?

LGTM.

Seems obvious.

Looks good but "to match 884acbb9e167d5668e43581630239d688edec8ad" might be more accurate since that was where the current definition of AllowInaccurateRcp was introduced.

Seems pretty obvious. Is it well defined which intrinsics are "legacy"? E.g. is it documented anywhere, or do we ever generate warnings for them or anything like that?

Well (fmul x, 0.0) -> 0.0 is still wrong if x is Inf or NaN.

I don't really follow the details but it looks like quite a nice simplification.

Use INSTRUCTION_LIST_END.

The point of this patch is to remove some unnecessary movs around the v_fma(c)_legacy_f32 instructions. I can pre-commit the new test case to make this clearer if you're happy with it.

LGTM. It is correct in more cases than it was before, and it mostly matches what the DAG version does.

I think this looks good, and we could even get rid of the AnyExtAsZext flag and do it unconditionally. Adding some globalisel folks to see if there are any objections.

Seems like an obvious refactoring to me. I think the API could be simplified though. How about returning all indices via the SmallVector, and returning a bool failure indication? Or say that if the vector is empty, it means failure?

This seems to cause a whole load of warnings during my Release build:

[1148/2221] Building RISCVGenAsmMatcher.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1793/2221] Building RISCVGenAsmWriter.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1795/2221] Building RISCVGenCompressInstEmitter.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1802/2221] Building RISCVGenRegisterBank.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1803/2221] Building RISCVGenRegisterInfo.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1805/2221] Building RISCVGenDAGISel.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1810/2221] Building RISCVGenGlobalISel.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.
[1813/2221] Building RISCVGenInstrInfo.inc...
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.

Sounds good to me.

I wouldn't have bothered with pre-commit review for a change like this.

Looks good, thanks!

Reverse ping! Was there a reason not to apply this?