Page MenuHomePhabricator

AMDGPU/GlobalISel/Emitter Recognize additional 'same operand checks'

Authored by Petar.Avramovic on Wed, Sep 9, 1:43 AM.



The "name" of a non-leaf complex pattern (MY_PAT $op1, $op2) is
"MY_PAT:op1:op2" and the ones with same "name" represent same operand.
Add 'same operand check' for this case.

Diff Detail

Event Timeline

Petar.Avramovic created this revision.Wed, Sep 9, 1:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 9, 1:43 AM
Petar.Avramovic requested review of this revision.Wed, Sep 9, 1:43 AM
foad added a comment.Wed, Sep 9, 1:56 AM

How does the new test case relate to the tablegen change? Perhaps you could pre-commit it so we can see the test case diffs?

Petar.Avramovic added a reviewer: paquette.

Show how this emitter change affects isel, and update tablegen tests.
Add additional checks for complex suboperands for test added in D57980. Neither tablegen nor sdag have check for such pattern and it looks like an error to me (there is no way for tablegen to know that some c++ code that generates two operands will generate same operand x (first out operand) from different input operands - these are operand 1 and 2 of G_ADD instruction). Also, if I checked correctly, there were no patterns like that in llvm upstream. Skip such patterns anyway in GlobalIselEmitter.

arsenm added a comment.Wed, Sep 9, 7:45 AM

Can you add tests for the min3/max3/med3 patterns? I think this is what was blocking those


Is this pattern really correct? It should probably be moved into a combine instead of a selection pattern if so

min3/max3 patterns are handled in combiner, inducing med3 patterns with constants. Patch imports med3 isel patterns: min(max(a, b), max(min(a, b), c)) but they need some combines/legalize fixes to be selected. From new imports only V_FRACT can be selected at the moment.


It is correctly selected since -enable-unsafe-fp-math flag was on.

arsenm accepted this revision.Fri, Sep 11, 2:02 PM
arsenm added inline comments.

I think this should be moved into a combine and not depend on the global flag, but that's a separate change

This revision is now accepted and ready to land.Fri, Sep 11, 2:02 PM