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

[Dominik Montada via Phabricator via llvm-commits]

gargaroff added inline comments.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4188
       }
       if (SrcChild->getOperator()->getName() == "timm") {
         OM.addPredicate<ImmOperandMatcher>();
----------------
dsanders wrote:
> 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.
Do you have some suggestion what could be done? Even if we remove the special handling here, at one point we will have to do at least some special handling because in one case we have to check for a `G_CONSTANT` while in the other we have to check for an immediate operand.

I guess the checker in match-table could technically do both dynamically: check one condition first and if that doesn't apply, check the other one. However I'm not sure what changes this would need in this emitter, as the predicate matcher for `G_CONSTANT` is an `InstructionMatcher` while for `timm` we need an `OperandMatcher`. I'm not too familiar with the internals of TableGen, so the only way to differentiate between the two known to me, is to check by name unfortunately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91703