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

Daniel Sanders via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 10 17:21:55 PST 2021


dsanders added inline comments.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4188
       }
       if (SrcChild->getOperator()->getName() == "timm") {
         OM.addPredicate<ImmOperandMatcher>();
----------------
arsenm wrote:
> dsanders wrote:
> > gargaroff wrote:
> > > 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.
> > For the checking by name bit, I was just saying that I don't really like using the name but there's not really a reasonable alternative in this case. We can leave it distinguishing them by name
> > 
> > For the imm/timm not corresponding to G_CONSTANT/isImm() bit, I was mostly acknowledging that something doesn't quite fit with the mapping from DAGISel to GlobalISel in general not really w.r.t this patch. The problem is that the distinction between Constant/TargetConstant is rather artificial, they're both constants that can match and be rendered as isImm() immediates and they can both be materialized in a register. The only distinction is what optimizations DAGISel is allowed to apply to them. I think the closest we can get in GlobalISel is that both imm and timm are allowed to match G_CONSTANT's, but timm is also allowed to match isImm() immediates as well. The one bit that still doesn't quite match up with DAGISel there is if you have an isImm() style immediate but no timm rules that match it, you'll get cannot-select whereas DAGISel would have materialized it.
> > 
> > Does that cover what you need?
> > The problem is that the distinction between Constant/TargetConstant is rather artificial, they're both constants that can match and be rendered as isImm() immediates and they can both be materialized in a register
> 
> No, this is the real and significant distinction. Constant is materialized in a register, TargetConstant must not be. Transforming a constant from a materialized one into an isImm operand is a function of the output operand transform
There's nothing I've seen in the definition of timm/TargetConstant that forbids materialization in a register. Implementation-wise they're both ConstantSDNode's and are only distinguishable by the opcode being either Constant or TargetConstant. This keeps them apart in the rules as DAGISel rules which check the opcode rather than isa<ConstantSDNode> but aside from having a different enum value in ISD enum they're functionally the same in DAGISel's implementation.

If it's true that timm never materializes for a given target then it's only by a lack of a `(set $dst, $src:timm)` pattern rather than by anything in the nature of timm itself. Maybe that's common enough that we can get away with enforcing it in the importer but it's not enforced in DAGISel as far as I've seen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91703



More information about the llvm-commits mailing list