[TableGen][GlobalISel] Fix handling of zero_reg

[ehjogab]

Summary

When generating matching tables for GlobalISel, TableGen would output
"::zero_reg" whenever encountering the zero_reg, which in turn would
result in compilation error. This patch fixes that by instead outputting
NoRegister (== 0), which is the same result that TableGen produces when
generating matching tables for ISelDAG.

[arsenm]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

Could this use TargetNamespace::NoRegister?

[ehjogab]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

I suppose that works. I checked for our target and NoRegister is indeed assigned value 0, so the semantics would be the same. But I am not sure whether that would work for all targets. If it does, I do believe your suggestion to be a better solution than to simply output value 0. Perhaps someone more knowledgeable regarding TableGen could comment on this?

[arsenm]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

That's generated for all targets. 0 is always $noreg, with an added NoRegister entry.

I also think zero_reg should really be renamed to $noreg or something closer to what it really is

[bjope]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

Agree zero_reg is not a good name if this refers to NoRegister (register number zero in the internal representation).

I would have guessed that it mapped to a register always being read as zero (if the target got such a register).

Changing the name is perhaps outside the scope of this patch. But we better (make sure) clarify that zero_reg is NoRegister (holding an undefined value, rather than holding the value zero).

[ehjogab]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

Looking into the Target/Target.td file where zero_reg is defined, it says the following:

/// zero_reg definition - Special node to stand for the zero register.
///
def zero_reg;

Is "zero register" and "NoRegister" really the same thing? If not, what should then be output?

[bjope]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

MIRLangRef also talks about "The null registers".

I think that TargetNamespace::NoRegister, zero_reg, null registers, _ (in MIR and dumps) and $noreg more or less all refer to the same thing - register number zero. But perhaps some targets could map a register always being read as zero to $noreg, e.g. translating that specifier to register number 31 in the asm printer.

It is confusing to have that many different names for the same thing. And to me zero_reg and null register is most confusing as there are targets that have registers always being read as zero (and the register number for such a register could be any number). So unless zero_reg actually is supposed to refer to a register being read as zero, it would be better to for example call it noreg or reg_null.

Looks like SystemZ got one use of zero_reg, and the rest belongs to ARM.

[ehjogab]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

Then I suppose we will continue outputting "NoRegister"; I just need to figure out how to retrieve the "NameSpace" from the zero_reg directive, since that's not immediately available...

[arsenm]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

Actually I think the code would work as is for this. If you look at CopyOrAddZeroRegRenderer,

<< MatchTable::NamedValue(
        (ZeroRegisterDef->getValue("Namespace")
             ? ZeroRegisterDef->getValueAsString("Namespace")
             : ""),

[ehjogab]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

I have looked into that but RegisterDef does not have "Namespace" in the case of zero_reg, so I need to retrieve the namespace information elsewhere.

[arsenm]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

So this check for the namespace is pointless and should be removed?

[ehjogab]

[TableGen][GlobalISel] Fix handling of zero_reg

When generating matching tables for GlobalISel, TableGen would output
"::zero_reg" whenever encountering the zero_reg, which in turn would
result in compilation error. This patch fixes that by instead outputting
NoRegister (== 0), which is the same result that TableGen produces when
generating matching tables for ISelDAG.

[ehjogab]

Forgot to update testcase.

[ehjogab]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2656

In the case of ZeroRegisterDef it seems to be a different story, as ZeroRegisterDef takes the value of GIZeroRegister whenever it is used, which could be any kind of register which may or may not have a Namespace. In the case of zero_reg, however, it always does not have a Namespace.

I have now updated the patch to look over the defs present in the target in search of a valid Namespace, so now we can use NoRegister (although the fix for this feels hacky, to me).

[bjope]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2670

Maybe one can add something like this to CodeGenTaget (putting the code close to the getInstNamespace support):

StringRef CodeGenTarget::getRegNamespace() const {
  auto &RegClasses = RegBank.getRegClasses();
  return RegClasses.size() > 0 ? RegClasses.front().Namespace : "";
}

and then use Target.getRegNamespace() to get it.

[ehjogab]

Updated patch according to bjope's suggestion.

[arsenm]

llvm/utils/TableGen/CodeGenTarget.cpp

549 ↗

(On Diff #287932)

Is using a different namespace from the overall target actually supported? Can't you just take the namespace from CodeGenTarget directly?

[bjope]

llvm/utils/TableGen/CodeGenTarget.cpp

547 ↗

(On Diff #287932)

Just like in the header file. I'd rather put this next to getInstNamespace. This would also show more clearly that we currently fetch the namespaces in different ways, and by that we perhaps support different namespaces for regs and instructions etc. I have no idea if that is used (or useful) in reality.

I figure that we at least wouldn't break anything as long as we follow what CodeGenRegisters is using when defining the enum with NoRegister, when fetching the getRegNamespace.

llvm/utils/TableGen/CodeGenTarget.h

194 ↗

(On Diff #287932)

I'd put this next to getInstNamespace(). Or if for some reason it would be better to keep "inst" helpers and "reg" helpers separated, then I'd put it next to something related to registers such as getRegBank or getRegRegisterByName. But I can't see any logic in the current order of methods (it's even mixing public and private declarations), so that's why I'd got for keeping the namespace-related functions close to each other.

[ehjogab]

Reordered functions as per bjope's feedback

[arsenm]

LGTM

[bjope]

I landed this on behalf of Gabriel.

llvm/test/TableGen/GlobalISelEmitter-zero-reg.td

+// RUN: llvm-tblgen -gen-global-isel -optimize-match-table=false -I %p/../../include -I %p/Common %s -o - < %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def P0 : Register<"p0"> { let Namespace = "MyTarget"; }
+def PR32 : RegisterClass<"MyTarget", [i32], 32, (add P0)>;
+def PR32Op : RegisterOperand<PR32>;
+
+def pred : PredicateOperand<OtherVT,
+                            (ops PR32:$FR),
+                            (ops (i32 zero_reg))> {}
+class PredI<dag OOps, dag IOps, list<dag> Pat>
+  : Instruction {
+  let Namespace = "MyTarget";
+  let OutOperandList = OOps;
+  let InOperandList = !con(IOps, (ins pred:$pred));
+  let Pattern = Pat;
+}
+
+def INST : PredI<(outs GPR32:$dst), (ins GPR32:$src), []>;
+
+// CHECK: GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_LOAD,
+// CHECK-NEXT: GIM_CheckMemorySizeEqualToLLT, /*MI*/0, /*MMO*/0, /*OpIdx*/0,
+// CHECK-NEXT: GIM_CheckAtomicOrdering, /*MI*/0, /*Order*/(int64_t)AtomicOrdering::NotAtomic,
+// CHECK-NEXT: // MIs[0] dst
+// CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT: // MIs[0] src
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/0, /*Op*/1, /*SizeInBits*/32,
+// CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT: // (ld:{ *:[i32] } GPR32:{ *:[i32] }:$src)<<P:Predicate_unindexedload>><<P:Predicate_load>>  =>  (INST:{ *:[i32] } GPR32:{ *:[i32] }:$src)
+// CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::INST,
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src
+// CHECK-NEXT: GIR_AddRegister, /*InsnID*/0, /*zero_reg*/0, /*AddRegisterRegFlags*/0,
+// CHECK-NEXT: GIR_MergeMemOperands, /*InsnID*/0, /*MergeInsnID's*/0, GIU_MergeMemOperands_EndOfList,
+// CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
+// CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+def : Pat<(i32 (load GPR32:$src)),
+          (INST GPR32:$src)>;

llvm/utils/TableGen/GlobalISelEmitter.cpp

 //===- GlobalISelEmitter.cpp - Generate an instruction selector -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
AddRegisterRenderer(unsigned InsnID, const Record *RegisterDef,
                       bool IsDef = false)
       : OperandRenderer(OR_Register), InsnID(InsnID), RegisterDef(RegisterDef),
         IsDef(IsDef) {}
 
   static bool classof(const OperandRenderer *R) {
     return R->getKind() == OR_Register;
   }
 
   void emitRenderOpcodes(MatchTable &Table, RuleMatcher &Rule) const override {
     Table << MatchTable::Opcode("GIR_AddRegister")
-          << MatchTable::Comment("InsnID") << MatchTable::IntValue(InsnID)
-          << MatchTable::NamedValue(
+          << MatchTable::Comment("InsnID") << MatchTable::IntValue(InsnID);
+    if (RegisterDef->getName() != "zero_reg") {
+      Table << MatchTable::NamedValue(
                    (RegisterDef->getValue("Namespace")
                         ? RegisterDef->getValueAsString("Namespace")
                         : ""),
-                 RegisterDef->getName())
-          << MatchTable::Comment("AddRegisterRegFlags");
+                   RegisterDef->getName());
+    } else {
+      Table << MatchTable::Comment("zero_reg");
+      Table << MatchTable::IntValue(0);
+    }
+    Table << MatchTable::Comment("AddRegisterRegFlags");
 
     // TODO: This is encoded as a 64-bit element, but only 16 or 32-bits are
     // really needed for a physical register reference. We can pack the
     // register and flags in a single field.
     if (IsDef)
       Table << MatchTable::NamedValue("RegState::Define");
     else
       Table << MatchTable::IntValue(0);
     Table << MatchTable::LineBreak;
   }
 };
 
 /// Adds a specific temporary virtual register to the instruction being built.
 /// This is used to chain instructions together when emitting multiple
 /// instructions.
 class TempRegRenderer : public OperandRenderer {
 protected:
   unsigned InsnID;
   unsigned TempRegID;
   const CodeGenSubRegIndex *SubRegIdx;