[PATCH] D86203: [GlobalISel][TableGen] Add handling of unannotated dst pattern ops

Tue Dec 15 13:14:03 PST 2020

dsanders added a comment.

> But since everyone else have had to do this for other targets, I suppose I will be forced to do the same.

I think it was mandatory to have them at one point and many targets have it because of that. This might be the first time I've seen a rule omit it.

> One thing that I've not understood is how GlobalISel is different from the legacy ISel here. Apparently it isn't necessary to annotate things with the legacy ISel today? So is legacy ISel doing the same thing that this patch suggests to do also for GlobalISel, or why do we suddenly need to update all patterns now (I mean, somehow it has worked fine in the past, right)?

The semantics for DAGISel are complicated and largely undocumented. There's quite a few features that only come up once or twice or where a small change can make a pattern more conventional and remove the need to implement something. The goal wasn't really to re-implement the whole thing but rather to aid bring up and eventually transition to GlobalISel.

The big difference that matters w.r.t this though is that GlobalISel has a concept of register banks that didn't exist in DAGISel. The matcher side treats ValueType as a wildcard for RegisterBanks that matches anything. That can be a problem for some targets and such targets need to avoid using it but for others it can be fine. The renderer will copy the operand and thereby preserve bank information that already exists but something (typically GIR_ConstrainSelectedInstOperands ) will have to constrain it to a class for ISel to have done its job.

> If annotations are needed, then maybe we need some kind of debug tool to find any inconsistencies? Or is there already an easy way to spot such things?

CodeGenDAGPatterns should object if the source and dest patterns disagree

> I assume tblgen will complain if it find something ambigous?

Maybe. It will try for some things but I wouldn't rely on that to be complete.

> Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

I think that's probably the right semantics to match DAGISel and if it is, then I feel that it ought to be CodeGenDAGPatterns's job to fill in the blanks. Have you found DAGISel's code for it?

>> But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?
>
> The problem isn't the type, it's the register class. It can be ambiguous to go from the target instruction back to the source bank if the target register class supports multiple banks

I think you may be talking cross-purposes here. It seems to me that there's two problems being discussed:

- What does the rule mean? I think filling in the blanks in the dst pattern using the values in the src pattern is the right semantics but I haven't dug into DAGISel to confirm that
- Is the resulting rule valid/importable? The answer will be target dependent according to the class->bank mappings. If it's unambiguous then I think accepting it is fine but if it's ambiguous (either by a single ambiguous class->multiple bank mapping, or by type->multiple class->multiple bank mapping) then we should reject the rule for that reason. That information should exist in RegisterBankEmitter.cpp, we might need to make it available to GlobalISelEmitter.cpp somehow

================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4161-4162
+getNumRegClassesInDag(const DagInit *Dag) {
+  std::string Op = Dag->getOperator()->getAsString();
+  if (Op.compare("add") == 0) {
+    unsigned Count = 0;
----------------
arsenm wrote:
> I think trying to parse this manually is problematic
I agree. That should be left to `SetTheory`, we should only be accessing register classes via `CodeGenRegisterClass` in this code

================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4281
+    if (InstrOpNodeRec) {
+      // If the record is a register class, check that it represents an
+      // allocatable class and is not a union of other register classes
----------------
I expect CodeGenRegisterClass has something for this. I haven't checked though

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86203