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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

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



================
Comment at: llvm/trunk/utils/TableGen/CodeGenRegisters.cpp:919
+Optional<std::pair<CodeGenRegisterClass *, CodeGenRegisterClass *>>
+CodeGenRegisterClass::getMatchingSubClassWithSubRegs(
+    CodeGenRegBank &RegBank, const CodeGenSubRegIndex *SubIdx) const {
----------------
>>Looks like we already have an escape hatch in TargetRegisterInfo::getSubClassWithSubReg. Should we just use that?
> That only selects a class suitable for the source of the copy. We also need to find a suitable class for the destination otherwise the MachineVerifier will complain that some destination registers for the subreg COPY are not subregisters of a register from the source class.


Huh, OK;  I thought we did have a helper somewhere (that computes the class of all subregisters of a superregister class via a given index).


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1669
+
+    if (DefInit *SubRegInit = dyn_cast<DefInit>(Dst->getChild(1)->getLeafValue())) {
+      CodeGenRegisterClass *RC = CGRegs.getRegClass(
----------------
There are multiple 80-col violations, btw


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1679
+        if (SrcRCDstRCPair->first != RC)
+          return failedImport("EXTRACT_SUBREG requires an additional COPY");
+      }
----------------
> 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);


```


================
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.
----------------
This was where I was suggesting an early-exit, not L1866: it might help to drop the if around SubRegInit?


================
Comment at: llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp:1884
+      if (DstIOpRec == nullptr)
+        return failedImport("EXTRACT_SUBREG operand #1 isn't a register class");
+
----------------
The operand number is also wrong, no? (here and elsewhere)


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


Repository:
  rL LLVM

https://reviews.llvm.org/D33596