User Details
- User Since
- Oct 29 2014, 9:58 AM (324 w, 5 d)
Today
Fri, Jan 15
LGTM.
"HasModifiers should not be true if at least one modifier is used."
LGTM, thanks!
Rebase.
Rebase. Pass Op0/1DemandedBits into SimplifyMultipleUseDemandedBits.
Thu, Jan 14
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.
Wed, Jan 13
LGTM. Any reason not to do this for SMAX and SMIN too?
This could do with an exhaustive unit test.
Tue, Jan 12
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?
Mon, Jan 11
LGTM with comment fix.
Looks OK to me. Does this enable any immediate simplifications? Or is it just to help with future work?
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
LGTM.
Fri, Jan 8
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?
Thu, Jan 7
LGTM.
Wed, Jan 6
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?
Tue, Jan 5
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.
Mon, Jan 4
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.
Wed, Dec 23
Dec 18 2020
LGTM. It is correct in more cases than it was before, and it mostly matches what the DAG version does.
Dec 17 2020
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.
Dec 16 2020
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.
Dec 15 2020
Dec 14 2020
Sounds good to me.
I wouldn't have bothered with pre-commit review for a change like this.
Dec 11 2020
Dec 10 2020
Looks good, thanks!
Reverse ping! Was there a reason not to apply this?