[Legalizer] Making artifact combining order-independent

Legalization algorithm is complicated by two facts:
1) While regular instructions should be possible to legalize in
   an isolated, per-instruction, context-free manner, legalization
   artifacts can only be eliminated in pairs, which could be deeply, and
   ultimately arbitrary nested: { [ () ] }, where which paranthesis kind
   depicts an artifact kind, like extend, unmerge, etc. Such structure
   can only be fully eliminated by simple local combines if they are
   attempted in a particular order (inside out), or alternatively by
   repeated scans each eliminating only one innermost pair, resulting in
   O(n^2) complexity.
2) Some artifacts might in fact be regular instructions that could (and
   sometimes should) be legalized by the target-specific rules. Which
   means failure to eliminate all artifacts on the first iteration is
   not a failure, they need to be tried as instructions, which may
   produce more artifacts, including the ones that are in fact regular
   instructions, resulting in a non-constant number of iterations
   required to finish the process.

I trust the recently introduced termination condition (no new artifacts
were created during as-a-regular-instruction-retrial of artifacts not
eliminated on the previous iteration) to be efficient in providing
termination, but only performing the legalization in full if and only if
at each step such chains of artifacts are successfully eliminated in
full as well.

Which is currently not guaranteed, as the artifact combines are applied
only once and in an arbitrary order that has to do with the order of
creation or insertion of artifacts into their worklist, which is a no
particular order.

In this patch I make a small change to the artifact combiner, making it
to re-insert into the worklist immediate (modulo a look-through copies)
artifact users of each vreg that changes its definition due to an
artifact combine.

Here the first scan through the artifacts worklist, while not
being done in any guaranteed order, only needs to find the innermost
pair(s) of artifacts that could be immediately combined out. After that
the process follows def-use chains, making them shorter at each step, thus
combining everything that can be combined in O(n) time.

Reviewers: volkan, aditya_nandakumar, qcolombet, paquette, aemerson, dsanders

Reviewed By: aditya_nandakumar, paquette

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D71448

Author: ramntry

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h

@@ -46,7 +46,8 @@ class LegalizationArtifactCombiner {
       : Builder(B), MRI(MRI), LI(LI) {}
 
   bool tryCombineAnyExt(MachineInstr &MI,
-                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                        SmallVectorImpl<MachineInstr *> &DeadInsts,
+                        SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_ANYEXT);
 
     Builder.setInstr(MI);
@@ -58,6 +59,7 @@ class LegalizationArtifactCombiner {
     if (mi_match(SrcReg, MRI, m_GTrunc(m_Reg(TruncSrc)))) {
       LLVM_DEBUG(dbgs() << ".. Combine MI: " << MI;);
       Builder.buildAnyExtOrTrunc(DstReg, TruncSrc);
+      UpdatedDefs.push_back(DstReg);
       markInstAndDefDead(MI, *MRI.getVRegDef(SrcReg), DeadInsts);
       return true;
     }
@@ -70,6 +72,7 @@ class LegalizationArtifactCombiner {
                                                     m_GSExt(m_Reg(ExtSrc)),
                                                     m_GZExt(m_Reg(ExtSrc)))))) {
       Builder.buildInstr(ExtMI->getOpcode(), {DstReg}, {ExtSrc});
+      UpdatedDefs.push_back(DstReg);
       markInstAndDefDead(MI, *ExtMI, DeadInsts);
       return true;
     }
@@ -83,15 +86,17 @@ class LegalizationArtifactCombiner {
         auto &CstVal = SrcMI->getOperand(1);
         Builder.buildConstant(
             DstReg, CstVal.getCImm()->getValue().sext(DstTy.getSizeInBits()));
+        UpdatedDefs.push_back(DstReg);
         markInstAndDefDead(MI, *SrcMI, DeadInsts);
         return true;
       }
     }
-    return tryFoldImplicitDef(MI, DeadInsts);
+    return tryFoldImplicitDef(MI, DeadInsts, UpdatedDefs);
   }
 
   bool tryCombineZExt(MachineInstr &MI,
-                      SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                      SmallVectorImpl<MachineInstr *> &DeadInsts,
+                      SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_ZEXT);
 
     Builder.setInstr(MI);
@@ -124,15 +129,17 @@ class LegalizationArtifactCombiner {
         auto &CstVal = SrcMI->getOperand(1);
         Builder.buildConstant(
             DstReg, CstVal.getCImm()->getValue().zext(DstTy.getSizeInBits()));
+        UpdatedDefs.push_back(DstReg);
         markInstAndDefDead(MI, *SrcMI, DeadInsts);
         return true;
       }
     }
-    return tryFoldImplicitDef(MI, DeadInsts);
+    return tryFoldImplicitDef(MI, DeadInsts, UpdatedDefs);
   }
 
   bool tryCombineSExt(MachineInstr &MI,
-                      SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                      SmallVectorImpl<MachineInstr *> &DeadInsts,
+                      SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_SEXT);
 
     Builder.setInstr(MI);
@@ -154,11 +161,12 @@ class LegalizationArtifactCombiner {
       markInstAndDefDead(MI, *MRI.getVRegDef(SrcReg), DeadInsts);
       return true;
     }
-    return tryFoldImplicitDef(MI, DeadInsts);
+    return tryFoldImplicitDef(MI, DeadInsts, UpdatedDefs);
   }
 
   bool tryCombineTrunc(MachineInstr &MI,
-                       SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                       SmallVectorImpl<MachineInstr *> &DeadInsts,
+                       SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_TRUNC);
 
     Builder.setInstr(MI);
@@ -174,6 +182,7 @@ class LegalizationArtifactCombiner {
         auto &CstVal = SrcMI->getOperand(1);
         Builder.buildConstant(
             DstReg, CstVal.getCImm()->getValue().trunc(DstTy.getSizeInBits()));
+        UpdatedDefs.push_back(DstReg);
         markInstAndDefDead(MI, *SrcMI, DeadInsts);
         return true;
       }
@@ -182,10 +191,10 @@ class LegalizationArtifactCombiner {
     return false;
   }
 
-
   /// Try to fold G_[ASZ]EXT (G_IMPLICIT_DEF).
   bool tryFoldImplicitDef(MachineInstr &MI,
-                          SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                          SmallVectorImpl<MachineInstr *> &DeadInsts,
+                          SmallVectorImpl<Register> &UpdatedDefs) {
     unsigned Opcode = MI.getOpcode();
     assert(Opcode == TargetOpcode::G_ANYEXT || Opcode == TargetOpcode::G_ZEXT ||
            Opcode == TargetOpcode::G_SEXT);
@@ -202,13 +211,15 @@ class LegalizationArtifactCombiner {
           return false;
         LLVM_DEBUG(dbgs() << ".. Combine G_ANYEXT(G_IMPLICIT_DEF): " << MI;);
         Builder.buildInstr(TargetOpcode::G_IMPLICIT_DEF, {DstReg}, {});
+        UpdatedDefs.push_back(DstReg);
       } else {
         // G_[SZ]EXT (G_IMPLICIT_DEF) -> G_CONSTANT 0 because the top
         // bits will be 0 for G_ZEXT and 0/1 for the G_SEXT.
         if (isConstantUnsupported(DstTy))
           return false;
         LLVM_DEBUG(dbgs() << ".. Combine G_[SZ]EXT(G_IMPLICIT_DEF): " << MI;);
         Builder.buildConstant(DstReg, 0);
+        UpdatedDefs.push_back(DstReg);
       }
 
       markInstAndDefDead(MI, *DefMI, DeadInsts);
@@ -269,7 +280,8 @@ class LegalizationArtifactCombiner {
   }
 
   bool tryCombineMerges(MachineInstr &MI,
-                        SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                        SmallVectorImpl<MachineInstr *> &DeadInsts,
+                        SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES);
 
     unsigned NumDefs = MI.getNumOperands() - 1;
@@ -319,8 +331,8 @@ class LegalizationArtifactCombiner {
           SmallVector<Register, 2> TmpRegs;
           // This is a vector that is being scalarized and casted. Extract to
           // the element type, and do the conversion on the scalars.
-          LLT MergeEltTy
-            = MRI.getType(MergeI->getOperand(0).getReg()).getElementType();
+          LLT MergeEltTy =
+              MRI.getType(MergeI->getOperand(0).getReg()).getElementType();
           for (unsigned j = 0; j < NumMergeRegs; ++j)
             TmpRegs.push_back(MRI.createGenericVirtualRegister(MergeEltTy));
 
@@ -331,6 +343,7 @@ class LegalizationArtifactCombiner {
         } else {
           Builder.buildUnmerge(DstRegs, MergeI->getOperand(Idx + 1).getReg());
         }
+        UpdatedDefs.append(DstRegs.begin(), DstRegs.end());
       }
 
     } else if (NumMergeRegs > NumDefs) {
@@ -352,7 +365,9 @@ class LegalizationArtifactCombiner {
              ++j, ++Idx)
           Regs.push_back(MergeI->getOperand(Idx).getReg());
 
-        Builder.buildMerge(MI.getOperand(DefIdx).getReg(), Regs);
+        Register DefReg = MI.getOperand(DefIdx).getReg();
+        Builder.buildMerge(DefReg, Regs);
+        UpdatedDefs.push_back(DefReg);
       }
 
     } else {
@@ -366,8 +381,9 @@ class LegalizationArtifactCombiner {
 
         for (unsigned Idx = 0; Idx < NumDefs; ++Idx) {
           Register MergeSrc = MergeI->getOperand(Idx + 1).getReg();
-          Builder.buildInstr(ConvertOp, {MI.getOperand(Idx).getReg()},
-                             {MergeSrc});
+          Register DefReg = MI.getOperand(Idx).getReg();
+          Builder.buildInstr(ConvertOp, {DefReg}, {MergeSrc});
+          UpdatedDefs.push_back(DefReg);
         }
 
         markInstAndDefDead(MI, *MergeI, DeadInsts);
@@ -378,9 +394,11 @@ class LegalizationArtifactCombiner {
              "Bitcast and the other kinds of conversions should "
              "have happened earlier");
 
-      for (unsigned Idx = 0; Idx < NumDefs; ++Idx)
-        MRI.replaceRegWith(MI.getOperand(Idx).getReg(),
-                           MergeI->getOperand(Idx + 1).getReg());
+      for (unsigned Idx = 0; Idx < NumDefs; ++Idx) {
+        Register NewDef = MergeI->getOperand(Idx + 1).getReg();
+        MRI.replaceRegWith(MI.getOperand(Idx).getReg(), NewDef);
+        UpdatedDefs.push_back(NewDef);
+      }
     }
 
     markInstAndDefDead(MI, *MergeI, DeadInsts);
@@ -399,7 +417,8 @@ class LegalizationArtifactCombiner {
   }
 
   bool tryCombineExtract(MachineInstr &MI,
-                         SmallVectorImpl<MachineInstr *> &DeadInsts) {
+                         SmallVectorImpl<MachineInstr *> &DeadInsts,
+                         SmallVectorImpl<Register> &UpdatedDefs) {
     assert(MI.getOpcode() == TargetOpcode::G_EXTRACT);
 
     // Try to use the source registers from a G_MERGE_VALUES
@@ -414,13 +433,14 @@ class LegalizationArtifactCombiner {
     // for N >= %2.getSizeInBits() / 2
     //    %3 = G_EXTRACT %1, (N - %0.getSizeInBits()
 
-    unsigned Src = lookThroughCopyInstrs(MI.getOperand(1).getReg());
-    MachineInstr *MergeI = MRI.getVRegDef(Src);
+    Register SrcReg = lookThroughCopyInstrs(MI.getOperand(1).getReg());
+    MachineInstr *MergeI = MRI.getVRegDef(SrcReg);
     if (!MergeI || !isMergeLikeOpcode(MergeI->getOpcode()))
       return false;
 
-    LLT DstTy = MRI.getType(MI.getOperand(0).getReg());
-    LLT SrcTy = MRI.getType(Src);
+    Register DstReg = MI.getOperand(0).getReg();
+    LLT DstTy = MRI.getType(DstReg);
+    LLT SrcTy = MRI.getType(SrcReg);
 
     // TODO: Do we need to check if the resulting extract is supported?
     unsigned ExtractDstSize = DstTy.getSizeInBits();
@@ -438,10 +458,9 @@ class LegalizationArtifactCombiner {
 
     // TODO: We could modify MI in place in most cases.
     Builder.setInstr(MI);
-    Builder.buildExtract(
-      MI.getOperand(0).getReg(),
-      MergeI->getOperand(MergeSrcIdx + 1).getReg(),
-      Offset - MergeSrcIdx * MergeSrcSize);
+    Builder.buildExtract(DstReg, MergeI->getOperand(MergeSrcIdx + 1).getReg(),
+                         Offset - MergeSrcIdx * MergeSrcSize);
+    UpdatedDefs.push_back(DstReg);
     markInstAndDefDead(MI, *MergeI, DeadInsts);
     return true;
   }
@@ -458,33 +477,79 @@ class LegalizationArtifactCombiner {
     // etc, process the dead instructions now if any.
     if (!DeadInsts.empty())
       deleteMarkedDeadInsts(DeadInsts, WrapperObserver);
+
+    // Put here every vreg that was redefined in such a way that it's at least
+    // possible that one (or more) of its users (immediate or COPY-separated)
+    // could become artifact combinable with the new definition (or the
+    // instruction reachable from it through a chain of copies if any).
+    SmallVector<Register, 4> UpdatedDefs;
+    bool Changed = false;
     switch (MI.getOpcode()) {
     default:
       return false;
     case TargetOpcode::G_ANYEXT:
-      return tryCombineAnyExt(MI, DeadInsts);
+      Changed = tryCombineAnyExt(MI, DeadInsts, UpdatedDefs);
+      break;
     case TargetOpcode::G_ZEXT:
-      return tryCombineZExt(MI, DeadInsts);
+      Changed = tryCombineZExt(MI, DeadInsts, UpdatedDefs);
+      break;
     case TargetOpcode::G_SEXT:
-      return tryCombineSExt(MI, DeadInsts);
+      Changed = tryCombineSExt(MI, DeadInsts, UpdatedDefs);
+      break;
     case TargetOpcode::G_UNMERGE_VALUES:
-      return tryCombineMerges(MI, DeadInsts);
+      Changed = tryCombineMerges(MI, DeadInsts, UpdatedDefs);
+      break;
     case TargetOpcode::G_EXTRACT:
-      return tryCombineExtract(MI, DeadInsts);
-    case TargetOpcode::G_TRUNC: {
-      if (tryCombineTrunc(MI, DeadInsts))
-        return true;
-
-      bool Changed = false;
-      for (auto &Use : MRI.use_instructions(MI.getOperand(0).getReg()))
-        Changed |= tryCombineInstruction(Use, DeadInsts, WrapperObserver);
-      return Changed;
+      Changed = tryCombineExtract(MI, DeadInsts, UpdatedDefs);
+      break;
+    case TargetOpcode::G_TRUNC:
+      Changed = tryCombineTrunc(MI, DeadInsts, UpdatedDefs);
+      if (!Changed) {
+        // Try to combine truncates away even if they are legal. As all artifact
+        // combines at the moment look only "up" the def-use chains, we achieve
+        // that by throwing truncates' users (with look through copies) into the
+        // ArtifactList again.
+        UpdatedDefs.push_back(MI.getOperand(0).getReg());
+      }
+      break;
     }
+    // If the main loop through the ArtifactList found at least one combinable
+    // pair of artifacts, not only combine it away (as done above), but also
+    // follow the def-use chain from there to combine everything that can be
+    // combined within this def-use chain of artifacts.
+    while (!UpdatedDefs.empty()) {
+      Register NewDef = UpdatedDefs.pop_back_val();
+      assert(NewDef.isVirtual() && "Unexpected redefinition of a physreg");
+      for (MachineInstr &Use : MRI.use_instructions(NewDef)) {
+        switch (Use.getOpcode()) {
+        // Keep this list in sync with the list of all artifact combines.
+        case TargetOpcode::G_ANYEXT:
+        case TargetOpcode::G_ZEXT:
+        case TargetOpcode::G_SEXT:
+        case TargetOpcode::G_UNMERGE_VALUES:
+        case TargetOpcode::G_EXTRACT:
+        case TargetOpcode::G_TRUNC:
+          // Adding Use to ArtifactList.
+          WrapperObserver.changedInstr(Use);
+          break;
+        case TargetOpcode::COPY: {
+          Register Copy = Use.getOperand(0).getReg();
+          if (Copy.isVirtual())
+            UpdatedDefs.push_back(Copy);
+          break;
+        }
+        default:
+          // If we do not have an artifact combine for the opcode, there is no
+          // point in adding it to the ArtifactList as nothing interesting will
+          // be done to it anyway.
+          break;
+        }
+      }
     }
+    return Changed;
   }
 
 private:
-
   static unsigned getArtifactSrcReg(const MachineInstr &MI) {
     switch (MI.getOpcode()) {
     case TargetOpcode::COPY:

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

@@ -212,6 +212,11 @@ Legalizer::legalizeMachineFunction(MachineFunction &MF, const LegalizerInfo &LI,
         // ArtifactCombiner to combine away them.
         if (isArtifact(MI)) {
           LLVM_DEBUG(dbgs() << ".. Not legalized, moving to artifacts retry\n");
+          assert(NumArtifacts == 0 &&
+                 "Artifacts are only expected in instruction list starting the "
+                 "second iteration, but each iteration starting second must "
+                 "start with an empty artifacts list");
+          (void)NumArtifacts;
           RetryList.push_back(&MI);
           continue;
         }
@@ -224,7 +229,7 @@ Legalizer::legalizeMachineFunction(MachineFunction &MF, const LegalizerInfo &LI,
     // Try to combine the instructions in RetryList again if there
     // are new artifacts. If not, stop legalizing.
     if (!RetryList.empty()) {
-      if (ArtifactList.size() > NumArtifacts) {
+      if (!ArtifactList.empty()) {
         while (!RetryList.empty())
           ArtifactList.insert(RetryList.pop_back_val());
       } else {

llvm/lib/Target/Mips/MipsRegisterBankInfo.cpp

@@ -651,8 +651,9 @@ void MipsRegisterBankInfo::setRegBank(MachineInstr &MI,
 static void
 combineAwayG_UNMERGE_VALUES(LegalizationArtifactCombiner &ArtCombiner,
                             MachineInstr &MI) {
+  SmallVector<Register, 4> UpdatedDefs;
   SmallVector<MachineInstr *, 2> DeadInstrs;
-  ArtCombiner.tryCombineMerges(MI, DeadInstrs);
+  ArtCombiner.tryCombineMerges(MI, DeadInstrs, UpdatedDefs);
   for (MachineInstr *DeadMI : DeadInstrs)
     DeadMI->eraseFromParent();
 }

llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll

@@ -167,7 +167,7 @@ define void @nonpow2_load_narrowing() {
   ret void
 }
 
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: cannot select: %5:fpr32(s32) = G_EXTRACT %{{[0-9]+}}:fpr(s128), 64 (in function: nonpow2_store_narrowing)
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: cannot select: %14:gpr64(s64), %15:gpr(s1) = G_UADDE %17:gpr, %17:gpr, %13:gpr (in function: nonpow2_store_narrowing)
 ; FALLBACK-WITH-REPORT-ERR: warning: Instruction selection used fallback path for nonpow2_store_narrowing
 ; FALLBACK-WITH-REPORT-OUT-LABEL: nonpow2_store_narrowing:
 define void @nonpow2_store_narrowing(i96* %c) {

llvm/test/CodeGen/AArch64/GlobalISel/legalize-merge-values.mir

@@ -15,9 +15,7 @@ body: |
     ; CHECK: [[AND1:%[0-9]+]]:_(s32) = G_AND [[COPY]], [[C1]]
     ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY [[SHL]](s32)
     ; CHECK: [[OR:%[0-9]+]]:_(s32) = G_OR [[AND1]], [[COPY1]]
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s8) = G_TRUNC [[OR]](s32)
-    ; CHECK: [[COPY2:%[0-9]+]]:_(s8) = COPY [[TRUNC]](s8)
-    ; CHECK: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[COPY2]](s8)
+    ; CHECK: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[OR]](s32)
     ; CHECK: $x0 = COPY [[ANYEXT]](s64)
     %0:_(s64) = G_CONSTANT i64 0
     %1:_(s4) = G_TRUNC %0