Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
410 ms | windows > lld.ELF/invalid::symtab-sh-info.s |
Event Timeline
Automatically folding legalization artifacts makes me a bit nervous. What happens if you want to legalize a constant by widening, and the legalizer wants to insert the trunc from a widened constant?
I don't know. I did wonder about that. It didn't seem to cause any problems in the test suite.
Are you suggesting that the existing BinOp folding is OK but only because it doesn't introduce constants of any new types, only of the same type as other constants that already exist?
This looks like something that might be an issue for us downstream, as we have pretty strict legalization rules. I could try to see if it actually causes issues for us. Is there anything I need to do to use the CSEMIRBuilder?
I think it's enabled by default for some passes, unless you override TargetPassConfig::isGISelCSEEnabled to return false, and enabled in the combiner only if you pass some CSEInfo into combineMachineInstrs, which no in-tree targets do.
I attempted this in the past but abandoned it due to infinite loops in legalization like mentioned earlier. Constant folding during legalization seems okay as long as it operates on legal types (ie fold an operation of constants (which are already legal) to the same type.
OK, perhaps I will have to abandon this too. But it would be nice to somehow get the codegen improvements shown in some of the MIPS and X86 tests.
I just tested the patch in our downstream implementation. While it doesn't cause any infinite loops in our regression tests, it seems to have caused our build bots to crash. I cannot attribute this 100% to this patch though. The crash reason is reported as running out of disk space, but checking all agents still have hundreds of gigs of free space, which is why I suspect that it ran out of memory due to the infinite loops instead. Especially since our nightly runs finished without any hick-ups just hours prior.
Unfortunately at the moment I cannot narrow this down further or even create a minimal example without some more time-consuming investigation.
Let me maybe add a suggestion: in my eyes the big problem here is that the combine that is introduced here can produce potentially illegal instructions, if run during / after the legalizer. So without a check whether the combine would produce something valid, I don't think it's a good idea to implement this.
The other combines are not problematic, since they don't change the types.
But it would be nice to somehow get the codegen improvements shown in some of the MIPS and X86 tests.
It looks like X86 and MIPS just need to run a post legalizer combiner pass.
clang-tidy: warning: invalid case style for function 'ConstantFoldUnOp' [readability-identifier-naming]
not useful