Page MenuHomePhabricator

Rework be15dfa88fb1 such that it works with GlobalISel which doesn't use EVT
ClosedPublic

Authored by dsanders on Jan 6 2020, 6:19 PM.

Details

Summary

be15dfa88fb1 broke GlobalISel's usage of getSetCCInverse() which currently
appears to be limited to our out-of-tree backend. GlobalISel doesn't use
EVT's and isn't able to derive them from the information it has as it
doesn't distinguish between integer and floating point types (that
distinction is made by operations rather than values). Bring back the
bool version of getSetCCInverse() in a way that doesn't break the intent
of be15dfa88fb1 but also allows GlobalISel to continue using it.

Diff Detail

Event Timeline

dsanders created this revision.Jan 6 2020, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 6:19 PM
dsanders updated this revision to Diff 236489.Jan 6 2020, 6:35 PM

Corrected patch

Could we invert the boolean flag to be isFloat? I fear that calling it isInteger, will lead the the same problems that I tried to fix in that commit (calling .isInteger() returns false for pointers).

Could we invert the boolean flag to be isFloat? I fear that calling it isInteger, will lead the the same problems that I tried to fix in that commit (calling .isInteger() returns false for pointers).

GlobalISel's LLT type doesn't have an isInteger() and distinguishes scalars from pointers so I think that's less of a risk for GlobalISel. For SelectionDAG, my guard against misuse is moving it to the GlobalISel namespace so that SelectionDAG code can't call it by accident.

That said, I don't mind inverting it in principle but one risk I see with that is that clangs fixits will direct downstream people to add the namespace but won't point out that the bool is inverted. We could name it isIntegral, isIntegerLike, isIntegerComparison, etc. if that helps.

Could we an overload that takes LLT instead and keep the boolean flag internal to the .cpp?

arichardson accepted this revision.Jan 9 2020, 3:01 PM
Could we an overload that takes `LLT` instead and keep the boolean flag internal to the .cpp?

Never mind, I saw now that LLT doesn't store int/float information. Seems fine to me but I either would either use isFloat or adjust the name of the boolean to isIntegerLike. Ideally also add a comment that pointers should use the integer variant.

This revision is now accepted and ready to land.Jan 9 2020, 3:01 PM
dsanders updated this revision to Diff 237402.Jan 10 2020, 11:37 AM

isInteger -> isIntegerLike and comment on pointers needing to use isIntegerLike

This revision was automatically updated to reflect the committed changes.