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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D33596#792538, @ab wrote:

> Can you add a tablegen testcase?   If nothing else, it's a good way to document the tablegen backend.


Sure.



================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1669
+
+    if (DefInit *SubRegInit = dyn_cast<DefInit>(Dst->getChild(1)->getLeafValue())) {
+      CodeGenRegisterClass *RC = CGRegs.getRegClass(
----------------
ab wrote:
> There are multiple 80-col violations, btw
Ok, I'll fix these. Hopefully, I'll also find time soon to get the commit hook I used to have working again.


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1679
+        if (SrcRCDstRCPair->first != RC)
+          return failedImport("EXTRACT_SUBREG requires an additional COPY");
+      }
----------------
ab wrote:
> > We need to compute it to determine whether we need to copy the source to a suitable register class or not. The current code deals with the case where the extra copy isn't needed and rejects the one that needs it. To support the case that needs it we'll need to refactor to support multiple insn emission.
> 
> 
> Sure, but wouldn't it work to only compute it once, later, when emitting, and check whether we need the copy then?   Here's a concrete patch:
> 
> 
> ```
>  utils/TableGen/GlobalISelEmitter.cpp | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git i/utils/TableGen/GlobalISelEmitter.cpp w/utils/TableGen/GlobalISelEmitter.cpp
> index 50da9085c21..edb772e5c26 100644
> --- i/utils/TableGen/GlobalISelEmitter.cpp
> +++ w/utils/TableGen/GlobalISelEmitter.cpp
> @@ -1667,18 +1667,8 @@ Expected<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
>        return failedImport("EXTRACT_SUBREG child #1 is not a leaf");
> 
>      if (DefInit *SubRegInit = dyn_cast<DefInit>(Dst->getChild(1)->getLeafValue())) {
> -      CodeGenRegisterClass *RC = CGRegs.getRegClass(
> -          getInitValueAsRegClass(Dst->getChild(0)->getLeafValue()));
>        CodeGenSubRegIndex *SubIdx = CGRegs.getSubRegIdx(SubRegInit->getDef());
> 
> -      const auto &SrcRCDstRCPair =
> -          RC->getMatchingSubClassWithSubRegs(CGRegs, SubIdx);
> -      if (SrcRCDstRCPair.hasValue()) {
> -        assert(SrcRCDstRCPair->second && "Couldn't find a matching subclass");
> -        if (SrcRCDstRCPair->first != RC)
> -          return failedImport("EXTRACT_SUBREG requires an additional COPY");
> -      }
> -
>        DstMIBuilder.addRenderer<CopySubRegRenderer>(
>            InsnMatcher, Dst->getChild(0)->getName(), SubIdx);
>        return DstMIBuilder;
> @@ -1898,6 +1888,8 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
>        const auto &SrcRCDstRCPair =
>            SrcRC->getMatchingSubClassWithSubRegs(CGRegs, SubIdx);
>        assert(SrcRCDstRCPair->second && "Couldn't find a matching subclass");
> +      if (SrcRCDstRCPair->first != SrcRC)
> +        return failedImport("EXTRACT_SUBREG requires an additional COPY");
>        M.addAction<ConstrainOperandToRegClassAction>("NewI", 0, *SrcRCDstRCPair->second);
>        M.addAction<ConstrainOperandToRegClassAction>("NewI", 1, *SrcRCDstRCPair->first);
> 
> 
> ```
That looks like it will work at the moment but I'm not sure it fits with the emission of nested instructions. For example:
  (EXTRACT_SUBREG (ADD x:s64, y:s64), sub_32)
could select to:
  %2(A64) = ADD %0(A64), %1(A64)
  %3(B64) = COPY %2(A64)
  %4(B32) = COPY %3(B64), sub_32
where `A64` and `B64` are disjoint register classes and `B32` is a subregister class of `B64`. Handling the extra copy outside the tree walking code would need a way to insert the copy into the right part of the emitted list. If we handle it during the tree walking code we can just append to the list as we go.

Here's a particularly complicated example from Mips:
  def : MSAPat<
    (i32 (vextract_zext_i8 v16i8:$ws, i64:$idx)),
    (SRL (COPY_TO_REGCLASS
           (i32 (EXTRACT_SUBREG
                   (SPLAT_B v16i8:$ws,
                     (COPY_TO_REGCLASS
                       (i32 (EXTRACT_SUBREG i64:$idx, sub_32)), GPR32)),
                   sub_lo)),
           GPR32),
         (i32 24))>;


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1878
+    if (DefInit *SubRegInit =
+            dyn_cast<DefInit>(Dst->getChild(1)->getLeafValue())) {
+      // Constrain the result to the same register bank as the operand.
----------------
ab wrote:
> This was where I was suggesting an early-exit, not L1866: it might help to drop the if around SubRegInit?
Ah I see. I don't mind either way, I'll make the change.


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1884
+      if (DstIOpRec == nullptr)
+        return failedImport("EXTRACT_SUBREG operand #1 isn't a register class");
+
----------------
ab wrote:
> The operand number is also wrong, no? (here and elsewhere)
It depends how you view the data. The gMIR view is:
  %0 = EXTRACT_SUBREG %1, subidx
in which case it's operand 1, but the DAG view is
  (EXTRACT_SUBREG %1, subidx)
in which case it's child 1. I've been using 'operand' for the gMIR view and 'child' for the DAG view.


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1896
+      //        actually contain the subregisters.
+      assert(Src->getExtTypes().size() == 1);
+
----------------
ab wrote:
> Add a message?
Ok


Repository:
  rL LLVM

https://reviews.llvm.org/D33596