[GlobalISel][Legalizer] Don't use eraseFromParentAndMarkDBGValuesForRemoval() for some artifacts.

[aemerson]

Summary

For artifacts excluding G_TRUNC/G_SEXT, which have IR counterparts, we don't
seem to have debug users of defs. However, in the legalizer we're always calling
MachineInstr::eraseFromParentAndMarkDBGValuesForRemoval() which is expensive.
In some rare cases, this contributes significantly to unreasonably long compile
times when we have lots of artifact combiner activity.

To verify this, I added asserts to that function when it actually replaced a debug
use operand with undef for these artifacts. On CTMark with both -O0 and -Os and
debug info enabled, I didn't see a single case where it triggered.

In my measurements I saw around a 0.5% geomean compile-time improvement on -g -O0
for AArch64 with this change.

[aemerson]

Fix test.

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185

Don't we already have an isArtifact check somewhere?

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185

Oh, I just reread and this is different. Can you add a comment justifying why these operations are skipped?

[arsenm]

Maybe should add a verifier check that these never have dbg users

[Petar.Avramovic]

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir

90

This is now broken, dbg_use has no def. Maybe delete this test since it was artificial to begin with. Relevant tests are above.

[aemerson]

In D109750#2999516, @arsenm wrote:

Maybe should add a verifier check that these never have dbg users

I don't think we should go that far. If we do have a debug user then not marking the use as undef AFAICT isn't strictly incorrect.

[aemerson]

Add comment.

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185

Should still get a comment explaining why these operations

[aemerson]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185

I updated very shortly before your reply. Is this comment ok?

[foad]

Needs rebasing (maybe re-benchmarking too?) after D109154.

[aemerson]

In D109750#3009146, @foad wrote:

Needs rebasing (maybe re-benchmarking too?) after D109154.

I think this is orthogonal to D109154 so don't need to rebenchmark.

[aemerson]

Rebase

[paquette]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1060 ↗

(On Diff #373724)

might as well pull the return false; into the default case?

[arsenm]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1060 ↗

(On Diff #373724)

I think the only combination that stops warnings on all compilers is return false in the default case with an llvm_unreachable after the switch

[dsanders]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058 ↗

(On Diff #373787)

I don't think this assumption holds beyond the first time the legalizer processes each instruction and is dubious earlier than that if you have pre-legalization passes too.

For the standard pipeline of passes, suppose you have an expansion in the legalizer that replaces some operation with a DBG_VALUE with something ending in G_MERGE_VALUES (or one of the others). On the next legalization step you have a G_MERGE_VALUES with a DBG_VALUE. That already breaks the assumption it doesn't have one but it takes a bit more to go wrong. If anything causes this G_MERGE_VALUES to be deleted without replacement you are left with a use-without-def as the DBG_VALUE is not erased. For example, if this is one lane of a bigger G_UNMERGE_VALUES/G_MERGE_VALUES pair and something proves the lane isn't needed it would erase all the instructions for the lane but leave the DBG_VALUE behind.

I haven't dug into it myself but I'm told we've run into this scenario in our downstream target and we've had to revert this change.

[jackoalan]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058 ↗

(On Diff #373787)

I'd like to echo this claim. It is far too presumptuous to assume artifacts like G_MERGE_VALUES are never created pre-legalizer for all targets. If anything, this decision should be made per-target in LegalizerInfo.

[jackoalan]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058 ↗

(On Diff #373787)

I found a specific case where GlobalISel's IRTranslator emits G_MERGE_VALUES: Lowering a call with arguments that are wider than native machine types. I think G_UNMERGE_VALUES, G_MERGE_VALUES, G_CONCAT_VECTORS, G_BUILD_VECTOR should be excluded from this test because they are all reachable in CallLowering and therefore originate from IR.

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

public:
 
   void changedInstr(MachineInstr &MI) override {
     // When insts change, we want to revisit them to legalize them again.
     // We'll consider them the same as created.
     LLVM_DEBUG(dbgs() << ".. .. Changed MI: " << MI);
     createdOrChangedInstr(MI);
   }
 };
+
+/// These artifacts generally don't have any debug users because they don't
+/// directly originate from IR instructions, but instead usually from
+/// legalization. Avoiding checking for debug users improves compile time.
+/// Note that truncates or extends aren't included because they have IR
+/// counterparts which can have debug users after translation.
+bool shouldSkipDbgValueFor(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  case TargetOpcode::G_UNMERGE_VALUES:
+  case TargetOpcode::G_MERGE_VALUES:
+  case TargetOpcode::G_CONCAT_VECTORS:
+  case TargetOpcode::G_BUILD_VECTOR:
+  case TargetOpcode::G_EXTRACT:
+  case TargetOpcode::G_INSERT:
+    return true;
+  }
+  return false;
+}
+
+void eraseInstr(MachineInstr &MI) {
+  if (shouldSkipDbgValueFor(MI))
+    MI.eraseFromParent();
+  else
+    MI.eraseFromParentAndMarkDBGValuesForRemoval();
+}
 } // namespace
 
 Legalizer::MFResult
 Legalizer::legalizeMachineFunction(MachineFunction &MF, const LegalizerInfo &LI,
                                    ArrayRef<GISelChangeObserver *> AuxObservers,
                                    LostDebugLocObserver &LocObserver,
                                    MachineIRBuilder &MIRBuilder) {
   MIRBuilder.setMF(MF);
do {
     assert(RetryList.empty() && "Expected no instructions in RetryList");
     unsigned NumArtifacts = ArtifactList.size();
     while (!InstList.empty()) {
       MachineInstr &MI = *InstList.pop_back_val();
       assert(isPreISelGenericOpcode(MI.getOpcode()) &&
              "Expecting generic opcode");
       if (isTriviallyDead(MI, MRI)) {
         LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
-        MI.eraseFromParentAndMarkDBGValuesForRemoval();
+        eraseInstr(MI);
         LocObserver.checkpoint(false);
         continue;
       }
 
       // Do the legalization for this instruction.
       auto Res = Helper.legalizeInstrStep(MI, LocObserver);
       // Error out if we couldn't legalize this instruction. We may want to
       // fall back to DAG ISel instead in the future.
do {
     LocObserver.checkpoint();
     while (!ArtifactList.empty()) {
       MachineInstr &MI = *ArtifactList.pop_back_val();
       assert(isPreISelGenericOpcode(MI.getOpcode()) &&
              "Expecting generic opcode");
       if (isTriviallyDead(MI, MRI)) {
         LLVM_DEBUG(dbgs() << MI << "Is dead\n");
         RemoveDeadInstFromLists(&MI);
-        MI.eraseFromParentAndMarkDBGValuesForRemoval();
+        eraseInstr(MI);
         LocObserver.checkpoint(false);
         continue;
       }
       SmallVector<MachineInstr *, 4> DeadInstructions;
       LLVM_DEBUG(dbgs() << "Trying to combine: " << MI);
       if (ArtCombiner.tryCombineInstruction(MI, DeadInstructions,
                                             WrapperObserver)) {
         WorkListObserver.printNewInstrs();
         for (auto *DeadMI : DeadInstructions) {
           LLVM_DEBUG(dbgs() << "Is dead: " << *DeadMI);
           RemoveDeadInstFromLists(DeadMI);
-          DeadMI->eraseFromParentAndMarkDBGValuesForRemoval();
+          eraseInstr(*DeadMI);
         }
         LocObserver.checkpoint(
             VerifyDebugLocs ==
             DebugLocVerifyLevel::LegalizationsAndArtifactCombiners);
         Changed = true;
         continue;
       }
       // If this was not an artifact (that could be combined away), this might

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir

bb.0:
     %8:_(<2 x s32>) = G_BUILD_VECTOR %6(s32), %7(s32)
     %9:_(<2 x s32>) = G_INSERT %8, %5(s32), 32
     %deaf_def:_(s32), %11:_(s32) = G_UNMERGE_VALUES %9(<2 x s32>)
     G_STORE %6(s32), %0(p5) :: (store (s32), align 8, addrspace 5)
     %12:_(s32) = G_CONSTANT i32 4
     %13:_(p5) = G_PTR_ADD %0, %12(s32)
     G_STORE %11(s32), %13(p5) :: (store (s32) into unknown-address + 4, addrspace 5)
 
 ...
-
----
-name: value_finder_bug_before_artifact_combine_dbg_use
-body: |
-  bb.0:
-    liveins: $vgpr0, $vgpr1, $vgpr2
-
-    ; GFX10-LABEL: name: value_finder_bug_before_artifact_combine_dbg_use
-    ; GFX10: [[COPY:%[0-9]+]]:_(p5) = COPY $vgpr0
-    ; GFX10: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
-    ; GFX10: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
-    ; GFX10: [[MV:%[0-9]+]]:_(p4) = G_MERGE_VALUES [[COPY1]](s32), [[COPY2]](s32)
-    ; GFX10: [[LOAD:%[0-9]+]]:_(<4 x s32>) = G_LOAD [[MV]](p4) :: (load (<4 x s32>), align 4, addrspace 4)
-    ; GFX10: [[EXTRACT:%[0-9]+]]:_(s32) = G_EXTRACT [[LOAD]](<4 x s32>), 96
-    ; GFX10: [[EXTRACT1:%[0-9]+]]:_(s32) = G_EXTRACT [[LOAD]](<4 x s32>), 64
-    ; GFX10: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
-    ; GFX10: DBG_VALUE $noreg, $noreg, {{.*}}, !DIExpression(), debug-location !DILocation(line: 1, column: 1
-    ; GFX10: G_STORE [[EXTRACT1]](s32), [[COPY]](p5) :: (store (s32), align 8, addrspace 5)
-    ; GFX10: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
-    ; GFX10: [[PTR_ADD:%[0-9]+]]:_(p5) = G_PTR_ADD [[COPY]], [[C]](s32)
-    ; GFX10: G_STORE [[EXTRACT]](s32), [[PTR_ADD]](p5) :: (store (s32) into unknown-address + 4, addrspace 5)
-    %0:_(p5) = COPY $vgpr0
-    %1:_(s32) = COPY $vgpr1
-    %2:_(s32) = COPY $vgpr2
-    %3:_(p4) = G_MERGE_VALUES %1(s32), %2(s32)
-    %4:_(<4 x s32>) = G_LOAD %3(p4) :: (load (<4 x s32>), align 4, addrspace 4)
-    %5:_(s32) = G_EXTRACT %4(<4 x s32>), 96
-    %6:_(s32) = G_EXTRACT %4(<4 x s32>), 64
-    %7:_(s32) = G_IMPLICIT_DEF
-    %8:_(<2 x s32>) = G_BUILD_VECTOR %6(s32), %7(s32)
-    %9:_(<2 x s32>) = G_INSERT %8, %5(s32), 32
-    %dbg_use:_(s32), %11:_(s32) = G_UNMERGE_VALUES %9(<2 x s32>)
-    DBG_VALUE %dbg_use(s32), $noreg, !7, !DIExpression(), debug-location !9
-    G_STORE %6(s32), %0(p5) :: (store (s32), align 8, addrspace 5)
-    %12:_(s32) = G_CONSTANT i32 4
-    %13:_(p5) = G_PTR_ADD %0, %12(s32)
-    G_STORE %11(s32), %13(p5) :: (store (s32) into unknown-address + 4, addrspace 5)
-...