[PATCH] D33596: [globalisel][tablegen] Add support for EXTRACT_SUBREG.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-trunc.mir:19
+# CHECK-NEXT:  - { id: 0, class: gpr64all }
+# CHECK-NEXT:  - { id: 1, class: gpr32sp }
 registers:
----------------
qcolombet wrote:
> Do you know why we end up with 'all' in one case and 'sp' in the other?
> I would have expected to both have 'all' or 'sp'.
The gpr32sp is indirectly specified by the rule:
   // (trunc:i32 GPR64sp:i64:$src)  =>  (EXTRACT_SUBREG:i32 GPR64sp:i64:$src, sub_32:i32)
$src is specified to be GPR64sp and the best fit for the sub_32 subregister index is GPR32sp. At this point, $src is left as GPR (which sounds wrong to me now that I've noticed it, I think it should constrain it to GPR64sp, I'll take another look at it). The GPR64all comes from the '%vreg0<def>(s64) = COPY %X0; GPR:%vreg0'



================
Comment at: utils/TableGen/CodeGenRegisters.h:368
+    /// SubRegClass and the largest subregister class that contains those
+    /// subregisters without (as far as possible) also containing additional registers.
+    ///
----------------
qcolombet wrote:
> The "as far as possible" part doesn't sound right. I thought tablegen was generating the missing register classes for that. 
It tries to find the best class (often one of the generated classes) but I'm not sure I can guarantee that it will always find it for all targets. AMDGPU's unusual use of subregister indices is what I had in mind since I think that may end up picking a subreg register class that contains additional registers. Additional registers in the subreg register class isn't a problem if it happens.


https://reviews.llvm.org/D33596