[GlobalISel] Correct comment about type vs register class

[dsandersllvm]

Type and register class aren't mutually exclusive in gMIR but target
instructions don't have types on their operands that have register classes.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

Author: Daniel Sanders (dsandersllvm)

Changes

Type and register class aren't mutually exclusive in gMIR but target
instructions don't have types on their operands that have register classes.

---

Full diff: https://github.com/llvm/llvm-project/pull/116083.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp (+10-9)

diff --git a/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp b/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
index fefa8f2ea85942..179ac9deb3d4a5 100644
--- a/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
@@ -147,6 +147,16 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
   unsigned Opcode = MI.getOpcode();
   LLT DstTy = MRI.getType(R);
 
+  // Handle the case where this is called on a register that does not have a
+  // type constraint (i.e. it's a target instruction with a register class
+  // constraint instead). This is unlikely to occur except by looking through
+  // copies but it is possible for the initial register being queried to be in
+  // this state.
+  if (!DstTy.isValid()) {
+    Known = KnownBits();
+    return;
+  }
+
 #ifndef NDEBUG
   if (DstTy.isFixedVector()) {
     assert(
@@ -158,15 +168,6 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
   }
 #endif
 
-  // Handle the case where this is called on a register that does not have a
-  // type constraint (i.e. it has a register class constraint instead). This is
-  // unlikely to occur except by looking through copies but it is possible for
-  // the initial register being queried to be in this state.
-  if (!DstTy.isValid()) {
-    Known = KnownBits();
-    return;
-  }
-
   unsigned BitWidth = DstTy.getScalarSizeInBits();
   auto CacheEntry = ComputeKnownBitsCache.find(R);
   if (CacheEntry != ComputeKnownBitsCache.end()) {

[arsenm]

Comment didn't change?

[dsandersllvm]

The change is in the parenthesis:

(i.e. it has a register class constraint instead)

changed to:

(i.e. it's a target instruction with a register class constraint instead)

[arsenm]

The word "instead" is the problematic part. Register class does not imply there is no LLT

[dsandersllvm]

I'll update to drop the word 'instead' but it's not the register class part that prevents the LLT. It's the 'target instruction' part. For example, the MOVi32imm in:

TEST_F(AArch64GISelMITest, TestKnownBitsCstWithClass) {
  StringRef MIRString = "  %10:gpr32 = MOVi32imm 1\n"
                        "  %4:_(s32) = COPY %10\n";
  setUp(MIRString);

LLT and Register Class appearing together happens in gMIR as you say. I think it can happen for certain kinds of pseudo-instructions too (those defined in a similar way to gMIR, we called them 'target-specific generic opcodes' at the time which is a terrible name but nobody had a better one 😀) but not for target instructions or pseudo-instructions defined like instructions

[arsenm]

You can still have a selected target instruction with a register class constraint, and an LLT. You can always have an LLT. All the LLTs are dropped only after selection is complete. AMDGPU requires the LLT plus the register classes to disambiguate boolean values

[dsandersllvm]

That could be true upstream nowadays although FWIW I don't think it was originally intended that way. The AArch64 tests in KnownBitsTest.cpp at least still mix target instructions without LLT's with gMIR that hasn't been selected yet.
Similarly, in our downstream target we have target instructions without LLT's amongst the gMIR/MIR from the legalizer onwards which is how the new DemandedElt's assertion first hit us. We had some code to conservatively derive DemandedBits from the register class but it was also causing it to ignore the accurate information in LLT's when they were present.

At this point, I think it's probably best to rephrase this comment more generally rather than just tweaking the counter example.

Maybe something like this?:

Handle the case where this is called on a register that does not have a type constraint. For example, it may be post-ISel or
this target might not preserve the type when early-selecting instructions.

[arsenm]

Even if you're introducing concrete register classes earlier than selection, it shouldn't hurt to add an LLT there

[dsandersllvm]

I agree, it might be worth making it a requirement for all targets