[PATCH] D156099: [AMDGPU] Add True16 register classes.

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir:31
   ; CHECK-NEXT:   [[DS_READ_B32_gfx9_:%[0-9]+]]:vgpr_32 = DS_READ_B32_gfx9 [[V_MOV_B32_e32_]], 0, 0, implicit $exec :: (load (s32), addrspace 3)
-  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 851978 /* regdef:VGPR_HI16 */, def %0, 2147549193 /* reguse tiedto:$1 */, %0(tied-def 3)
-  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 851977 /* reguse:VGPR_HI16 */, [[DS_READ_B32_gfx9_]]
-  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 851978 /* regdef:VGPR_HI16 */, def undef %0.sub0, 851978 /* regdef:VGPR_HI16 */, def undef %0.sub1
+  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 851978 /* regdef:VGPR_16 */, def %0, 2147549193 /* reguse tiedto:$1 */, %0(tied-def 3)
+  ; CHECK-NEXT:   INLINEASM &"", 1 /* sideeffect attdialect */, 851977 /* reguse:VGPR_16 */, [[DS_READ_B32_gfx9_]]
----------------
kosarev wrote:
> arsenm wrote:
> > Joe_Nash wrote:
> > > arsenm wrote:
> > > > Joe_Nash wrote:
> > > > > Unrelated to this patch, but I don't understand the purpose of these tests. It looks like we are referencing register classes by number in the test input. Therefore any time we modify the number of register classes the tests changes their meaning.
> > > > The raw number thing is a syntactical inconvenience from how inlineasm is represented. Ideally the mir printer/parser would produce something nicer. I think the purpose of the test is reasonably clear from the comment at the top
> > > How can one determine if the change in output is still correct? The different RC choices in coalescer-early-clobber-subreg.mir are even more abstract. Is the RC to be ignored?
> > the printed class name should be correct, you just get the added bonus random number. Fur the purpose of this test the class doesn't really matter. However there are no 16-bit values in this test, so I don't understand how this ended up with any vgpr16 references in the first place
> The InlineAsm instruction uses the numeric register class ids when computing these flags for register operands. These ids may change as we add or remove register classes. D78088 made the meaning of these flags visible in MIR tests, but there is still no support on the parser side for such operands.
> These ids may change as we add or remove register classes.

Exactly. 
The IDs in the input have not been updated but the output has been auto-updated about 10 times when someone previously added/removed a register class. Originally I think the register class was VRegOrLds_32. That is why we see 16-bit register classes in a test without 16-bit values. IMO this test format is not maintainable.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156099