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

Tue Mar 23 05:57:02 PDT 2021

gargaroff added inline comments.

================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4188
       }
       if (SrcChild->getOperator()->getName() == "timm") {
         OM.addPredicate<ImmOperandMatcher>();
----------------
dsanders wrote:
> dsanders wrote:
> > 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.
> > 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.
> 
> Just to clarify the last bit of that, if is true via the lack of a materialization pattern for all targets then I'll be ok with the importer only accepting isImm() immediates for when `timm` is used. I think we should probably verify the materialization pattern for `timm` doesn't exist and error if we find it though.
I tried to pick up this patch again but quickly realized that I'm still confused about what actually needs to be done  now. Should the code handling `timm`s be moved to some other place where it makes more sense? Should the cases for immediates in the instruction selector be merged? Or should the whole immediate handling be changed in some way?

I have never worked with DAG ISel and I don't know what the difference between `imm` and `timm` is supposed to be in the end. In my understanding it was just the distinction that `imm` maps to `G_CONSTANT` operand, while `timm` maps to an immediate operand, nothing more. From your discussion it seems like there is something more to this that would actually require a much bigger rework in this patch. Is that correct?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91703