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

[dsanders]

Summary

As of this patch, 350 out of 3938 rules are currently imported.

Depends on D32229

[dsanders]

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

Does anyone else have a preference?

[dsanders]

include/llvm/Target/GlobalISel/SelectionDAGCompat.td

36–37

It turns out these two lines aren't entirely true. In AArch64 they're used as operators in some contexts and operands in others. I'll drop these lines.

[ab]

LGTM

In 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?)

[dsanders]

In D32275#732358, @ab wrote:

LGTM

Thanks.

In 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?

[ab]

In D32275#732465, @dsanders wrote:

In D32275#732358, @ab wrote:

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?

Yeah, otherwise I'm guessing the generated tests stop covering everything as soon as something else is added, unless we automate it somehow. (so the main actionable thing would really be "generate tests for the new stuff")

[dsanders]

I decided to have a look at code coverage differences before committing this. This patch made no difference to the code coverage. On closer inspection it turns out beginFunction() isn't being executed so no rules with rule predicates can match. I'll take a look at this, maybe something from the rule-predicates patch didn't make it into the final commit.

A few other notable things in the coverage information:

• There's no coverage for 64-bit shifts.
• There's no coverage for 8 or 16-bit FPU stores. That's not particularly surprising but I am surprised that there's an implementation for those sizes.
• There's no coverage of 128-bit FPU copies
• Most integer and FPU comparison predicates aren't covered.
• There's no coverage for LOAD_STACK_GUARD
• There's no coverage for G_TRUNC in the FPU register bank.

[rovka]

In D32275#736537, @dsanders wrote:

I decided to have a look at code coverage differences before committing this. This patch made no difference to the code coverage. On closer inspection it turns out beginFunction() isn't being executed so no rules with rule predicates can match. I'll take a look at this, maybe something from the rule-predicates patch didn't make it into the final commit.

I literally just ran into this while trying to enable TableGen for ARM. I'm wondering if it's really a good idea to have beginFunction in the first place. There's a related FIXME in TargetSubtargetInfo.h around getInstructionSelector. Maybe we should have an instruction selector per MachineFunction?

A few other notable things in the coverage information:

• There's no coverage for 64-bit shifts.
• There's no coverage for 8 or 16-bit FPU stores. That's not particularly surprising but I am surprised that there's an implementation for those sizes.
• There's no coverage of 128-bit FPU copies
• Most integer and FPU comparison predicates aren't covered.
• There's no coverage for LOAD_STACK_GUARD
• There's no coverage for G_TRUNC in the FPU register bank.

[dsanders]

In D32275#736557, @rovka wrote:

In D32275#736537, @dsanders wrote:

I decided to have a look at code coverage differences before committing this. This patch made no difference to the code coverage. On closer inspection it turns out beginFunction() isn't being executed so no rules with rule predicates can match. I'll take a look at this, maybe something from the rule-predicates patch didn't make it into the final commit.

I literally just ran into this while trying to enable TableGen for ARM. I'm wondering if it's really a good idea to have beginFunction in the first place. There's a related FIXME in TargetSubtargetInfo.h around getInstructionSelector. Maybe we should have an instruction selector per MachineFunction?

I'm having another go at that approach at the moment. Last time I had some trouble with it using the wrong instruction selector during instruction selection but hopefully I'll figure that one out this time. The tricky predicate is NotWin64WithoutFP from X86 which needs access to the MachineFunction that getSubtargetImpl() doesn't have.

include/llvm/Target/GlobalISel/SelectionDAGCompat.td

 // Declare that a generic Instruction is 'equivalent' to an SDNode, that is,
 // SelectionDAG patterns involving the SDNode can be transformed to match the
 // Instruction instead.
 class GINodeEquiv<Instruction i, SDNode node> {
   Instruction I = i;
   SDNode Node = node;
 }
 
-def : GINodeEquiv<G_ZEXT, zext>;
+// These are defined in the same order as the G_* instructions.
+def : GINodeEquiv<G_ANYEXT, anyext>;
 def : GINodeEquiv<G_SEXT, sext>;
+def : GINodeEquiv<G_ZEXT, zext>;
+def : GINodeEquiv<G_TRUNC, trunc>;
+def : GINodeEquiv<G_BITCAST, bitconvert>;
+// G_INTTOPTR - SelectionDAG has no equivalent.
+// G_PTRTOINT - SelectionDAG has no equivalent.
+// G_CONSTANT - Not needed since constants aren't operators.
+// G_FCONSTANT - Not needed since constants aren't operators.
 def : GINodeEquiv<G_ADD, add>;
 def : GINodeEquiv<G_SUB, sub>;
 def : GINodeEquiv<G_MUL, mul>;
-
+def : GINodeEquiv<G_SDIV, sdiv>;
+def : GINodeEquiv<G_UDIV, udiv>;
+def : GINodeEquiv<G_SREM, srem>;
+def : GINodeEquiv<G_UREM, urem>;
+def : GINodeEquiv<G_AND, and>;
 def : GINodeEquiv<G_OR, or>;
 def : GINodeEquiv<G_XOR, xor>;
-def : GINodeEquiv<G_AND, and>;
-
 def : GINodeEquiv<G_SHL, shl>;
 def : GINodeEquiv<G_LSHR, srl>;
 def : GINodeEquiv<G_ASHR, sra>;
-
-def : GINodeEquiv<G_SDIV, sdiv>;
-def : GINodeEquiv<G_UDIV, udiv>;
-def : GINodeEquiv<G_SREM, srem>;
-def : GINodeEquiv<G_UREM, urem>;
-
+def : GINodeEquiv<G_SELECT, select>;
+def : GINodeEquiv<G_FNEG, fneg>;
+def : GINodeEquiv<G_FPEXT, fpextend>;
+def : GINodeEquiv<G_FPTRUNC, ftrunc>;
+def : GINodeEquiv<G_FPTOSI, fp_to_sint>;
+def : GINodeEquiv<G_FPTOUI, fp_to_uint>;
+def : GINodeEquiv<G_SITOFP, sint_to_fp>;
+def : GINodeEquiv<G_UITOFP, uint_to_fp>;
+def : GINodeEquiv<G_FADD, fadd>;
+def : GINodeEquiv<G_FSUB, fsub>;
+def : GINodeEquiv<G_FMUL, fmul>;
+def : GINodeEquiv<G_FDIV, fdiv>;
+def : GINodeEquiv<G_FREM, frem>;
+def : GINodeEquiv<G_FPOW, fpow>;
 def : GINodeEquiv<G_BR, br>;
 
 // Specifies the GlobalISel equivalents for SelectionDAG's ComplexPattern.
 // Should be used on defs that subclass GIComplexOperandMatcher<>.
 class GIComplexPatternEquiv<ComplexPattern seldag> {
   ComplexPattern SelDAGEquivalent = seldag;
 }