[PATCH] D32275: [globalisel][tablegen] Add several GINodeEquiv's for operators that do not require additional support.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

LGTM

In https://reviews.llvm.org/D32275#731752, @dsanders wrote:

> One thing to mention for this patch is that G_ICMP and G_FCMP are not included due to a difference in operand order between G_ICMP/G_FCMP in GlobalISel and setcc in SelectionDAG (predicate, lhs, rhs vs lhs, rhs, predicate). I can support those in a later patch by adding more information to GINodeEquiv or by changing G_ICMP/G_FCMP.


Eh, I mildly prefer changing G_ICMP, to avoid making tablegen even more complicated.  But that's not the main issue ...

> I don't see a particularly strong argument either way on that point but I'm expecting to need to convert the predicate operand too so I'm leaning towards making GINodeEquiv more descriptive.

... because the predicates are different, right?  In general, setcc/br_cc and friends are IMO at the limit of what we should support in the importer, but I suppose it could be worth it if it's not too involved.

I'm *terrified* by the lack of testing: we have no idea what changes and what's covered.  But that's true for all tablegen changes, and I'm even more worried about sdag pattern changes affecting gisel.
We should really at least generate the test inputs for the covered patterns, and also have a test that fails when we add more (maybe something trivial like the number of emitted patterns?)


https://reviews.llvm.org/D32275