[PATCH] D91703: [GISel] Teach TableGen to check predicates of immediate operands in patterns

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:782
     }
+    case GIM_CheckI64ImmArgPredicate: {
+      int64_t InsnID = MatchTable[CurrentIdx++];
----------------
arsenm wrote:
> gargaroff wrote:
> > foad wrote:
> > > gargaroff wrote:
> > > > arsenm wrote:
> > > > > Should probably group with    case GIM_CheckI64ImmPredicate: {
> > > > > 
> > > > > The name similarity is a bit confusing too
> > > > Do you have a suggestion for a better name? I'm struggling to come up with a different one. The only other idea I have is `TImmPredicate`, which is just as similar.
> > > "CheckI64ImmOperandPredicate"? Though there are plenty of other checks that check specific operands, that don't have "Operand" in their names.
> > I'd be fine with that. @arsenm ?
> MachineOperand calls these "isImm" and "isCImm", so these names should probably mirror that. I'm not sure about including "I64"
I think the main thing is including 'Operand' in the name but I do think the comments need to make the distinction clear. In GlobalISel very few immediates are operands. Normally, the operand is a vreg that's def'd by a G_CONSTANT instruction.

IIRC, the I64 comes from IntImmLeaf isAPInt==0. There was a lot of code that only dealt with <=64-bit and used constants so those convert to int64_t before running the custom C++ predicate. A few needed full APInt's though. I'd be inclined to keep the `I64` as sharing code with GIM_CheckI64ImmPredicate means the custom C++ predicate shares the same interface.

> MachineOperand calls these "isImm" and "isCImm", so these names should probably mirror that. I'm not sure about including "I64"

isImm() vs isCImm() isn't related to imm/timm. It's about the size of the immediate
```
   /// isImm - Tests if this is a MO_Immediate operand.
   /// isCImm - Test if this is a MO_CImmediate operand.
```
```
     MO_Immediate,         ///< Immediate operand
     MO_CImmediate,        ///< Immediate >64bit operand
```


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:278-280
+      assert((State.MIs[InsnID]->getOpcode() == TargetOpcode::G_CONSTANT ||
+              State.MIs[InsnID]->getOperand(OpIdx).isImm()) &&
+             "Expected G_CONSTANT or immediate operand");
----------------
This assert probably ought to be re-worked a bit as at first glance it appears to be always true. I think it still mostly works but does so indirectly because G_CONSTANT always has an isCImm() (to have type information) and never an isImm().

However, an immediate operand can be isImm() or isCImm() which may give you a false assert for the immediate operands case.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4188
       }
       if (SrcChild->getOperator()->getName() == "timm") {
         OM.addPredicate<ImmOperandMatcher>();
----------------
Ah ok, I see why `timm` is special, it's been special-cased and isn't handled with the other immediates. I'm a little surprised to see this here as this block of code is supposed to be handling MBB's. We should probably revise the comment on 4180 if this is correct. FWIW, I'd generally prefer that we didn't special case by name but I don't really have an alternative to suggest in this case.

What was the reason timm's needed to be handled here rather than with the `imm` and the other `ImmLeaf`'s? I'd hazard a guess that it's down to the distinction between having an `MCOperand` with `isImm() == true` and the normal `G_CONSTANT` but even if that's the reason, `timm` vs `imm` doesn't really correspond to that distinction (`timm` is just an `imm` that can't be folded/simplified/lowered) which suggests there's something else not quite right.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91703/new/

https://reviews.llvm.org/D91703