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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D32275#732358, @ab wrote:

> LGTM


Thanks.

> 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.

That's right. LLVM-IR uses CmpInst::Predicate and SelectionDAG uses ISD::CondCode. MIR may differ from both or may use something else like opcodes.

On the br_cc side of things, there's also an operator mismatch between GlobalISel and SelectionDAG on some targets caused by the lowering of BRCOND+SETCC to BR_CC. The rules are still importable but tablegen would have to undo the lowering and it's probably not worth it.

> 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,

We could potentially diff code coverage reports to find out which parts of the C++ implementation are ignored because of new tablegenerated rules.

> and I'm even more worried about sdag pattern changes affecting gisel.

It would be fairly easy to add a way to disable the importer for some patterns. Eventually, I'd expect to dump the imported rules in whatever format GlobalISel defines its rules and use that as the input with the importer disabled.

> We should really at least generate the test inputs for the covered patterns,

This is on my todo list. I had some experimental patches a while back but they'll need a lot of work.

> and also have a test that fails when we add more (maybe something trivial like the number of emitted patterns?)

I'm not sure I understand this bit. Is the intent to prompt us to look at the new rules?


https://reviews.llvm.org/D32275