Skip to content

Commit

Permalink
Only guard loop metadata that has non-debug info in it (llvm#118825)
Browse files Browse the repository at this point in the history
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:
1. Commit llvm@c07e19b adds loop
metadata including debug locations to loops that otherwise would not
have loop metadata
2. Commit llvm@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:
```C
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.
  • Loading branch information
dominik-steenken authored Dec 20, 2024
1 parent 54665f5 commit fa9cef5
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 22 deletions.
4 changes: 4 additions & 0 deletions llvm/include/llvm/IR/Instruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 hasNonDebugLocLoopMetadata() const;

/// Return true if this instruction has metadata attached to it other than a
/// debug location.
bool hasMetadataOtherThanDebugLoc() const { return Value::hasMetadata(); }
Expand Down
25 changes: 25 additions & 0 deletions llvm/lib/IR/Instruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@

#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"
#include "llvm/IR/InstrTypes.h"
#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"
Expand Down Expand Up @@ -461,6 +463,29 @@ bool Instruction::hasPoisonGeneratingMetadata() const {
hasMetadata(LLVMContext::MD_align);
}

bool Instruction::hasNonDebugLocLoopMetadata() 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);
Expand Down
13 changes: 8 additions & 5 deletions llvm/lib/Transforms/Utils/Local.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1279,10 +1279,10 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
// | for.body <---- (md2)
// |_______| |______|
if (Instruction *TI = BB->getTerminator())
if (TI->hasMetadata(LLVMContext::MD_loop))
if (TI->hasNonDebugLocLoopMetadata())
for (BasicBlock *Pred : predecessors(BB))
if (Instruction *PredTI = Pred->getTerminator())
if (PredTI->hasMetadata(LLVMContext::MD_loop))
if (PredTI->hasNonDebugLocLoopMetadata())
return false;

if (BBKillable)
Expand Down Expand Up @@ -1345,12 +1345,15 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
}
}

// If the unconditional branch we replaced contains llvm.loop metadata, we
// add the metadata to the branch instructions in the predecessors.
// If the unconditional branch we replaced contains non-debug llvm.loop
// metadata, we add the metadata to the branch instructions in the
// predecessors.
if (Instruction *TI = BB->getTerminator())
if (MDNode *LoopMD = TI->getMetadata(LLVMContext::MD_loop))
if (TI->hasNonDebugLocLoopMetadata()) {
MDNode *LoopMD = TI->getMetadata(LLVMContext::MD_loop);
for (BasicBlock *Pred : predecessors(BB))
Pred->getTerminator()->setMetadata(LLVMContext::MD_loop, LoopMD);
}

if (BBKillable) {
// Everything that jumped to BB now goes to Succ.
Expand Down
86 changes: 69 additions & 17 deletions llvm/test/Transforms/SimplifyCFG/preserve-llvm-loop-metadata.ll
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ if.then: ; preds = %while.body
br label %if.end

; CHECK: if.then:
; CHECK: br label %while.cond, !llvm.loop !0
; CHECK: br label %while.cond, !llvm.loop !1

if.else: ; preds = %while.body
%4 = load i32, ptr %count, align 4
Expand All @@ -37,10 +37,10 @@ if.else: ; preds = %while.body
br label %if.end

; CHECK: if.else:
; CHECK: br label %while.cond, !llvm.loop !0
; CHECK: br label %while.cond, !llvm.loop !1

if.end: ; preds = %if.else, %if.then
br label %while.cond, !llvm.loop !0
br label %while.cond, !llvm.loop !1

while.end: ; preds = %while.cond
ret void
Expand Down Expand Up @@ -74,7 +74,7 @@ entry:
br label %while.cond

while.cond.loopexit: ; preds = %for.body
br label %while.cond, !llvm.loop !2
br label %while.cond, !llvm.loop !3

while.cond: ; preds = %while.cond.loopexit, %entry
%i.0 = phi i32 [ %a, %entry ], [ %add, %while.cond.loopexit ]
Expand All @@ -96,22 +96,74 @@ for.body: ; preds = %while.body, %for.bo
%1 = tail call i32 asm sideeffect "add ${0:w}, ${1:w}\0A", "=r,r,~{cc}"(i32 %0)
%inc = add nuw nsw i32 %k.07, 1
%cmp1 = icmp ult i32 %inc, 5
br i1 %cmp1, label %for.body, label %while.cond.loopexit, !llvm.loop !4
br i1 %cmp1, label %for.body, label %while.cond.loopexit, !llvm.loop !5

while.end: ; preds = %while.cond
%sum.0.lcssa = phi i32 [ %sum.0, %while.cond ]
ret i32 %sum.0.lcssa
}

!0 = distinct !{!0, !1}
!1 = !{!"llvm.loop.distribute.enable", i1 true}
!2 = distinct !{!2, !3}
!3 = !{!"llvm.loop.mustprogress"}
!4 = distinct !{!4, !3, !5}
!5 = !{!"llvm.loop.unroll.enable"}
; CHECK: !0 = distinct !{!0, !1}
; CHECK: !1 = !{!"llvm.loop.distribute.enable", i1 true}
; CHECK: !2 = distinct !{!2, !3}
; CHECK: !3 = !{!"llvm.loop.mustprogress"}
; CHECK: !4 = distinct !{!4, !3, !5}
; CHECK: !5 = !{!"llvm.loop.unroll.enable"}
; Test that the condition tested above does not trigger when the loop metadata consists only of debug locations,
; i.e.the empty loop latch `while.cond.loopexit` *will* be folded into its successor if its
; predecessor blocks are also loop latches and any loop metadata attached to it consists of debug information.
;
define i32 @test3(i32 %a, i32 %b, i32 %step, i32 %remainder, ptr %input) !dbg !7 {
entry:
br label %while.cond

;CHECK-LABEL: @test3(
;CHECK-NOT: while.cond.loopexit
while.cond.loopexit: ; preds = %for.body
br label %while.cond, !llvm.loop !10

while.cond: ; preds = %while.cond.loopexit, %entry
%i.0 = phi i32 [ %a, %entry ], [ %add, %while.cond.loopexit ]
%sum.0 = phi i32 [ 0, %entry ], [ %1, %while.cond.loopexit ]
%sub = sub nsw i32 %b, %i.0
%cmp = icmp sgt i32 %sub, %remainder
br i1 %cmp, label %while.body, label %while.end

while.body: ; preds = %while.cond
%add = add nsw i32 %i.0, %step
br label %for.body

for.body: ; preds = %while.body, %for.body
%k.07 = phi i32 [ 0, %while.body ], [ %inc, %for.body ]
%add2 = add nsw i32 %k.07, %add
%idxprom = sext i32 %add2 to i64
%arrayidx = getelementptr inbounds i32, ptr %input, i64 %idxprom
%0 = load i32, ptr %arrayidx, align 4
%1 = tail call i32 asm sideeffect "add ${0:w}, ${1:w}\0A", "=r,r,~{cc}"(i32 %0)
%inc = add nuw nsw i32 %k.07, 1
%cmp1 = icmp ult i32 %inc, 5
br i1 %cmp1, label %for.body, label %while.cond.loopexit, !llvm.loop !5

while.end: ; preds = %while.cond
%sum.0.lcssa = phi i32 [ %sum.0, %while.cond ]
ret i32 %sum.0.lcssa
}

!llvm.module.flags = !{!0}

!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = distinct !{!1, !2}
!2 = !{!"llvm.loop.distribute.enable", i1 true}
!3 = distinct !{!3, !4}
!4 = !{!"llvm.loop.mustprogress"}
!5 = distinct !{!5, !4, !6}
!6 = !{!"llvm.loop.unroll.enable"}
!7 = distinct !DISubprogram(name: "test3", scope: !8, file: !8, spFlags: DISPFlagDefinition, unit: !9)
!8 = !DIFile(filename: "preserve-llvm-loop-metadata.ll", directory: "/")
!9 = distinct !DICompileUnit(language: DW_LANG_C99, file: !8, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
!10 = distinct !{!10, !11, !13}
!11 = !DILocation(line: 8, column: 4, scope: !12)
!12 = distinct !DILexicalBlock(scope: !7, file: !8, line: 8, column: 2)
!13 = !DILocation(line: 9, column: 23, scope: !12)

; CHECK: !1 = distinct !{!1, !2}
; CHECK: !2 = !{!"llvm.loop.distribute.enable", i1 true}
; CHECK: !3 = distinct !{!3, !4}
; CHECK: !4 = !{!"llvm.loop.mustprogress"}
; CHECK: !5 = distinct !{!5, !4, !6}
; CHECK: !6 = !{!"llvm.loop.unroll.enable"}
; CHECK-NOT: !10 = distinct !{!10, !11, !13}

0 comments on commit fa9cef5

Please sign in to comment.