This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Fold unary opcodes in CSEMIRBuilder
AbandonedPublic

Authored by foad on Oct 14 2020, 6:57 AM.

Diff Detail

Unit TestsFailed

Event Timeline

foad created this revision.Oct 14 2020, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 6:57 AM
foad requested review of this revision.Oct 14 2020, 6:57 AM

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?

foad added a comment.Oct 14 2020, 7:41 AM

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?

foad added a comment.Oct 14 2020, 7:51 AM

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.

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.

Understood, thanks. I'll try to test this tomorrow and will report back.

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.

foad added a comment.Oct 15 2020, 1:07 AM

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.

foad abandoned this revision.Oct 15 2020, 1:19 AM

Thanks for testing @gargaroff.

The consensus seems to be that this is not safe.

foad added a comment.Oct 15 2020, 1:53 AM

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.