Only guard loop metadata that has non-debug info in it #118825

dominik-steenken · 2024-12-05T15:59:20Z

This PR is motivated by a mismatch we discovered between compilation results with vs. without -g3. We noticed this when compiling SPEC2017 testcases. The specific instance we saw is fixed in this PR by modifying a guard (see below), but it is likely similar instances exist elsewhere in the codebase.

The specific case fixed in this PR manifests itself in the SimplifyCFG pass doing different things depending on whether DebugInfo is generated or not. At the end of this comment, there is reduced example code that shows the behavior in question.

The differing behavior has two root causes:

Commit c07e19b adds loop metadata including debug locations to loops that otherwise would not have loop metadata
Commit ac28efa6c100 adds a guard to a simplification action in SImplifyCFG that prevents it from simplifying away loop metadata

So, the change in 2. does not consider that when compiling with debug symbols, loops that otherwise would not have metadata that needs preserving, now have debug locations in their loop metadata. Thus, with -g3, SimplifyCFG behaves differently than without it.

The larger issue is that while debug info is not supposed to influence the final compilation result, commits like 1. blur the line between what is and is not debug info, and not all optimization passes account for this.

This PR does not address that and rather just modifies this particular guard in order to restore equivalent behavior between debug and non-debug builds in this one instance.

Here is a reduced version of a file from f526.blender_r that showcases the behavior in question:

struct LinkNode;
typedef struct LinkNode {
 struct LinkNode *next;
 void *link;
} LinkNode;

void do_projectpaint_thread_ph_v_state() {
  int *ps = do_projectpaint_thread_ph_v_state;
  LinkNode *node;
  while (do_projectpaint_thread_ph_v_state)
    for (node = ps; node; node = node->next)
      ;
}

Compiling this with and without DebugInfo, and then disassembling the results, leads to different outcomes (tested on SystemZ and X86). The reason for this is that the SimplifyCFG pass does different things in either case.

llvmbot · 2024-12-05T16:00:00Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Dominik Steenken (dominik-steenken)

Changes

This PR is motivated by a mismatch we discovered between compilation results with vs. without -g3. We noticed this when compiling SPEC2017 testcases. The specific instance we saw is fixed in this PR by modifying a guard (see below), but it is likely similar instances exist elsewhere in the codebase.

The specific case fixed in this PR manifests itself in the SimplifyCFG pass doing different things depending on whether DebugInfo is generated or not. At the end of this comment, there is reduced example code that shows the behavior in question.

The differing behavior has two root causes:

Commit c07e19b adds loop metadata including debug locations to loops that otherwise would not have loop metadata
Commit ac28efa6c100 adds a guard to a simplification action in SImplifyCFG that prevents it from simplifying away loop metadata

So, the change in 2. does not consider that when compiling with debug symbols, loops that otherwise would not have metadata that needs preserving, now have debug locations in their loop metadata. Thus, with -g3, SimplifyCFG behaves differently than without it.

The larger issue is that while debug info is not supposed to influence the final compilation result, commits like 1. blur the line between what is and is not debug info, and not all optimization passes account for this.

This PR does not address that and rather just modifies this particular guard in order to restore equivalent behavior between debug and non-debug builds in this one instance.

Here is a reduced version of a file from f526.blender_r that showcases the behavior in question:

struct LinkNode;
typedef struct LinkNode {
 struct LinkNode *next;
 void *link;
} LinkNode;

void do_projectpaint_thread_ph_v_state() {
  int *ps = do_projectpaint_thread_ph_v_state;
  LinkNode *node;
  while (do_projectpaint_thread_ph_v_state)
    for (node = ps; node; node = node-&gt;next)
      ;
}

Compiling this with and without DebugInfo, and then disassembling the results, leads to different outcomes (tested on SystemZ and X86). The reason for this is that the SimplifyCFG pass does different things in either case.

Full diff: https://github.com/llvm/llvm-project/pull/118825.diff

3 Files Affected:

(modified) llvm/include/llvm/IR/Instruction.h (+4)
(modified) llvm/lib/IR/Instruction.cpp (+25)
(modified) llvm/lib/Transforms/Utils/Local.cpp (+1-1)

diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 730baa8cc00520..ea3d93d856efb7 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -367,6 +367,10 @@ class Instruction : public User,
   /// Return true if this instruction has any metadata attached to it.
   bool hasMetadata() const { return DbgLoc || Value::hasMetadata(); }
 
+  // Return true if this instruction contains loop metadata other than
+  // a debug location
+  bool hasLoopMetadataOtherThanDebugLoc() const;
+
   /// Return true if this instruction has metadata attached to it other than a
   /// debug location.
   bool hasMetadataOtherThanDebugLoc() const { return Value::hasMetadata(); }
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 0137bb281e7ec5..6c19712481ca9a 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -12,6 +12,7 @@
 
 #include "llvm/IR/Instruction.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/AttributeMask.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Constants.h"
@@ -19,6 +20,7 @@
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MemoryModelRelaxationAnnotations.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
@@ -461,6 +463,29 @@ bool Instruction::hasPoisonGeneratingMetadata() const {
          hasMetadata(LLVMContext::MD_align);
 }
 
+bool Instruction::hasLoopMetadataOtherThanDebugLoc() const {
+  // If there is no loop metadata at all, we also don't have
+  // non-debug loop metadata, obviously.
+  if  (!hasMetadata(LLVMContext::MD_loop))
+    return false;
+
+  // If we do have loop metadata, retrieve it.
+  MDNode * LoopMD = getMetadata(LLVMContext::MD_loop);
+
+  // Check if the existing operands are debug locations. This loop
+  // should terminate after at most three iterations. Skip
+  // the first item because it is a self-reference.
+  for (const MDOperand& Op : llvm::drop_begin(LoopMD->operands())) {
+    // check for debug location type by attempting a cast.
+    if (!dyn_cast<DILocation>(Op)) {
+      return true;
+    }
+  }
+
+  // If we get here, then all we have is debug locations in the loop metadata.
+  return false;
+}
+
 void Instruction::dropPoisonGeneratingMetadata() {
   eraseMetadata(LLVMContext::MD_range);
   eraseMetadata(LLVMContext::MD_nonnull);
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index cdc3f0308fe59c..876017e9051067 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1279,7 +1279,7 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
   // |    for.body <---- (md2)
   // |_______|  |______|
   if (Instruction *TI = BB->getTerminator())
-    if (TI->hasMetadata(LLVMContext::MD_loop))
+    if (TI->hasLoopMetadataOtherThanDebugLoc())
       for (BasicBlock *Pred : predecessors(BB))
         if (Instruction *PredTI = Pred->getTerminator())
           if (PredTI->hasMetadata(LLVMContext::MD_loop))

llvmbot · 2024-12-05T16:00:00Z

@llvm/pr-subscribers-llvm-ir

Author: Dominik Steenken (dominik-steenken)

Changes

This PR is motivated by a mismatch we discovered between compilation results with vs. without -g3. We noticed this when compiling SPEC2017 testcases. The specific instance we saw is fixed in this PR by modifying a guard (see below), but it is likely similar instances exist elsewhere in the codebase.

The specific case fixed in this PR manifests itself in the SimplifyCFG pass doing different things depending on whether DebugInfo is generated or not. At the end of this comment, there is reduced example code that shows the behavior in question.

The differing behavior has two root causes:

Commit c07e19b adds loop metadata including debug locations to loops that otherwise would not have loop metadata
Commit ac28efa6c100 adds a guard to a simplification action in SImplifyCFG that prevents it from simplifying away loop metadata

So, the change in 2. does not consider that when compiling with debug symbols, loops that otherwise would not have metadata that needs preserving, now have debug locations in their loop metadata. Thus, with -g3, SimplifyCFG behaves differently than without it.

The larger issue is that while debug info is not supposed to influence the final compilation result, commits like 1. blur the line between what is and is not debug info, and not all optimization passes account for this.

This PR does not address that and rather just modifies this particular guard in order to restore equivalent behavior between debug and non-debug builds in this one instance.

Here is a reduced version of a file from f526.blender_r that showcases the behavior in question:

struct LinkNode;
typedef struct LinkNode {
 struct LinkNode *next;
 void *link;
} LinkNode;

void do_projectpaint_thread_ph_v_state() {
  int *ps = do_projectpaint_thread_ph_v_state;
  LinkNode *node;
  while (do_projectpaint_thread_ph_v_state)
    for (node = ps; node; node = node-&gt;next)
      ;
}

Compiling this with and without DebugInfo, and then disassembling the results, leads to different outcomes (tested on SystemZ and X86). The reason for this is that the SimplifyCFG pass does different things in either case.

Full diff: https://github.com/llvm/llvm-project/pull/118825.diff

3 Files Affected:

(modified) llvm/include/llvm/IR/Instruction.h (+4)
(modified) llvm/lib/IR/Instruction.cpp (+25)
(modified) llvm/lib/Transforms/Utils/Local.cpp (+1-1)

diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 730baa8cc00520..ea3d93d856efb7 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -367,6 +367,10 @@ class Instruction : public User,
   /// Return true if this instruction has any metadata attached to it.
   bool hasMetadata() const { return DbgLoc || Value::hasMetadata(); }
 
+  // Return true if this instruction contains loop metadata other than
+  // a debug location
+  bool hasLoopMetadataOtherThanDebugLoc() const;
+
   /// Return true if this instruction has metadata attached to it other than a
   /// debug location.
   bool hasMetadataOtherThanDebugLoc() const { return Value::hasMetadata(); }
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 0137bb281e7ec5..6c19712481ca9a 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -12,6 +12,7 @@
 
 #include "llvm/IR/Instruction.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/AttributeMask.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Constants.h"
@@ -19,6 +20,7 @@
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MemoryModelRelaxationAnnotations.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
@@ -461,6 +463,29 @@ bool Instruction::hasPoisonGeneratingMetadata() const {
          hasMetadata(LLVMContext::MD_align);
 }
 
+bool Instruction::hasLoopMetadataOtherThanDebugLoc() const {
+  // If there is no loop metadata at all, we also don't have
+  // non-debug loop metadata, obviously.
+  if  (!hasMetadata(LLVMContext::MD_loop))
+    return false;
+
+  // If we do have loop metadata, retrieve it.
+  MDNode * LoopMD = getMetadata(LLVMContext::MD_loop);
+
+  // Check if the existing operands are debug locations. This loop
+  // should terminate after at most three iterations. Skip
+  // the first item because it is a self-reference.
+  for (const MDOperand& Op : llvm::drop_begin(LoopMD->operands())) {
+    // check for debug location type by attempting a cast.
+    if (!dyn_cast<DILocation>(Op)) {
+      return true;
+    }
+  }
+
+  // If we get here, then all we have is debug locations in the loop metadata.
+  return false;
+}
+
 void Instruction::dropPoisonGeneratingMetadata() {
   eraseMetadata(LLVMContext::MD_range);
   eraseMetadata(LLVMContext::MD_nonnull);
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index cdc3f0308fe59c..876017e9051067 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1279,7 +1279,7 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
   // |    for.body <---- (md2)
   // |_______|  |______|
   if (Instruction *TI = BB->getTerminator())
-    if (TI->hasMetadata(LLVMContext::MD_loop))
+    if (TI->hasLoopMetadataOtherThanDebugLoc())
       for (BasicBlock *Pred : predecessors(BB))
         if (Instruction *PredTI = Pred->getTerminator())
           if (PredTI->hasMetadata(LLVMContext::MD_loop))

dominik-steenken · 2024-12-05T16:00:02Z

@uweigand @JonPsson1 fyi

github-actions · 2024-12-05T16:02:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JonPsson1 · 2024-12-05T20:18:24Z

Needs a test.

llvm/lib/IR/Instruction.cpp

dominik-steenken · 2024-12-18T11:07:13Z

While writing the test, i discovered an issue with the transform as it exists now with this change. I will need to update this further.

…etadata The transform TryToSimplifyUncondBranchFromEmptyBlock in SimplifyCFG contains a guard that is intended to prevent loop latches of nested loops that might have relevant loop metadata attached to them from being folded away, thus potentially destroying the metadata of the predecessor loop latch. This guard overprotects loop latches though, as in the case of compiling with debug information, loop metadata can be created that only contains debug location information, and doesn't carry any design intent. This commit changes the guard in such a way that metadata like this no longer protects against the loop latch from being folded away. The debug-only loop metadata is dropped in such a case.

dominik-steenken · 2024-12-19T12:15:25Z

I have added an additional check that makes sure that when we optimize away a loop latch with only debug info, that that debug info then isn't used to overwrite otherwise useful metadata in the predecessor.
I also expanded upon the existing tests.

llvmbot added llvm:ir llvm:transforms labels Dec 5, 2024

dominik-steenken force-pushed the only-guard-non-trivial-loopinfo branch from 71e8f49 to dc84ae0 Compare December 5, 2024 16:05

dominik-steenken force-pushed the only-guard-non-trivial-loopinfo branch 2 times, most recently from db64757 to 8ec819e Compare December 16, 2024 12:53

nikic added the debuginfo label Dec 16, 2024

dwblaikie reviewed Dec 16, 2024

View reviewed changes

llvm/lib/IR/Instruction.cpp Outdated Show resolved Hide resolved

dominik-steenken force-pushed the only-guard-non-trivial-loopinfo branch from 424daad to e4f2a4e Compare December 18, 2024 09:13

dominik-steenken force-pushed the only-guard-non-trivial-loopinfo branch 2 times, most recently from 0b6f693 to 84c9f6f Compare December 19, 2024 10:15

dominik-steenken force-pushed the only-guard-non-trivial-loopinfo branch from 84c9f6f to d4d11a7 Compare December 19, 2024 10:20

dwblaikie approved these changes Dec 19, 2024

View reviewed changes

uweigand merged commit fa9cef5 into llvm:main Dec 20, 2024
8 checks passed

dominik-steenken deleted the only-guard-non-trivial-loopinfo branch March 5, 2025 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Only guard loop metadata that has non-debug info in it #118825

Only guard loop metadata that has non-debug info in it #118825

Uh oh!

dominik-steenken commented Dec 5, 2024

Uh oh!

llvmbot commented Dec 5, 2024 •

edited

Loading

Uh oh!

llvmbot commented Dec 5, 2024

Uh oh!

dominik-steenken commented Dec 5, 2024

Uh oh!

github-actions bot commented Dec 5, 2024 •

edited

Loading

Uh oh!

JonPsson1 commented Dec 5, 2024

Uh oh!

Uh oh!

dominik-steenken commented Dec 18, 2024

Uh oh!

dominik-steenken commented Dec 19, 2024

Uh oh!

Uh oh!

Uh oh!

Only guard loop metadata that has non-debug info in it #118825

Only guard loop metadata that has non-debug info in it #118825

Uh oh!

Conversation

dominik-steenken commented Dec 5, 2024

Uh oh!

llvmbot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 5, 2024

Uh oh!

dominik-steenken commented Dec 5, 2024

Uh oh!

github-actions bot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonPsson1 commented Dec 5, 2024

Uh oh!

Uh oh!

dominik-steenken commented Dec 18, 2024

Uh oh!

dominik-steenken commented Dec 19, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Dec 5, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited

Loading