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

Daniel Sanders via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 02:41:31 PDT 2017


dsanders marked 5 inline comments as done.
dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:953
     if (DstRB.getID() != SrcRB.getID()) {
       DEBUG(dbgs() << "G_TRUNC input/output on different banks\n");
       return false;
----------------
This code is also used by G_PTRTOINT. I'll fix these comments.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:976-977
         // Nothing to be done
       } else if (DstRC == &AArch64::GPR32RegClass &&
                  SrcRC == &AArch64::GPR64RegClass) {
         I.getOperand(1).setSubReg(AArch64::sub_32);
----------------
I've updated this to account for the change you requested but I couldn't make that exact change due to a couple problems.

It turns out this is needed for G_PTRTOINT to prevent select-int-ptr-casts.mir failing. This is because a suitable rule isn't imported. As far as I can see there are no suitable rules to import.

It also turned out to be needed for G_TRUNC to support s1/s8/s16 to avoid errors like:
    LLVM ERROR: cannot select: %vreg1<def>(s8) = G_TRUNC %vreg0; GPR32:%vreg1 GPR64:%vreg0 (in function: trunc_s8_s64)
This happens because getRegClassForTypeOnBank() returns GPR32 for integer types smaller than 32 bit. To fix this, I've done the equivalent with DstTy and SrcTy.


================
Comment at: utils/TableGen/CodeGenRegisters.cpp:923
 
+CodeGenRegisterClass *CodeGenRegisterClass::getMatchingSubClassWithSubRegs(
+    CodeGenRegBank &RegBank, const CodeGenSubRegIndex *SubIdx,
----------------
ab wrote:
> 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.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:871
+  const StringRef SymbolicName;
+  // The subregister to extract.
+  const CodeGenSubRegIndex *SubReg;
----------------
ab wrote:
> Triple-slash.
Well spotted


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1625-1630
+      CodeGenRegisterClass *DstRC = nullptr;
+      CodeGenRegisterClass *SrcRC =
+          RC->getMatchingSubClassWithSubRegs(CGRegs, SubIdx, &DstRC);
+      assert(DstRC && "Couldn't find a matching subclass");
+      if (SrcRC != RC)
+        return failedImport("EXTRACT_SUBREG requires an additional COPY");
----------------
ab wrote:
> At this point, we don't need to compute SrcRC/DstRC, right?  Can you check this when emitting the constraining code?
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.


https://reviews.llvm.org/D33596





More information about the llvm-commits mailing list