[PATCH] Disable branch folding with MemorySanitizer

[Daniel Berlin]

Ignoring the question of whether this is a good thing to be doing for
a second, is there a reason to do this so deep in the folding logic?
Have you really evaluated all the other simplifications being
performed by SimplifyUncondBranch and SimplifyCondBranch and
determined they don't have similar issues for you?

Otherwise, it seems like this check belongs around here:


 Builder.SetInsertPoint(BB->getTerminator());
   if (BranchInst *BI = dyn_cast<BranchInst>(BB->getTerminator())) {
     if (BI->isUnconditional()) {
       if (SimplifyUncondBranch(BI, Builder)) return true;
     } else {
<check goes here>

       if (SimplifyCondBranch(BI, Builder)) return true;
     }

Rather than all the way down in the code that does the actual folding

On Mon, Nov 18, 2013 at 7:02 AM, Evgeniy Stepanov <eugenis at google.com> wrote:
> Branch folding optimization often leads to confusing MSan reports due to lost debug info.
> For example,
> 1: if (x < 0)
> 2:   if (y < 0)
> 3:    do_something();
> is transformed into something like
>   %0 = and i32 %y, %x
>   %1 = icmp slt i32 %0, 0
>   br i1 %1, label %if.then2, label %if.end3
> where all 3 instructions are associated with line 1.
>
> This patch disables folding of conditional branches in functions with sanitize_memory attribute.
>
> http://llvm-reviews.chandlerc.com/D2214
>
> Files:
>   lib/Transforms/Utils/SimplifyCFG.cpp
>   test/Transforms/SimplifyCFG/branch-fold-msan.ll
>
> Index: lib/Transforms/Utils/SimplifyCFG.cpp
> ===================================================================
> --- lib/Transforms/Utils/SimplifyCFG.cpp
> +++ lib/Transforms/Utils/SimplifyCFG.cpp
> @@ -1967,6 +1967,13 @@
>  bool llvm::FoldBranchToCommonDest(BranchInst *BI) {
>    BasicBlock *BB = BI->getParent();
>
> +  // This optimization results in confusing MemorySanitizer reports. Use of
> +  // uninitialized value in this branch instruction is reported with the
> +  // predecessor's debug location.
> +  if (BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory) &&
> +      BI->isConditional())
> +    return false;
> +
>    Instruction *Cond = 0;
>    if (BI->isConditional())
>      Cond = dyn_cast<Instruction>(BI->getCondition());
> Index: test/Transforms/SimplifyCFG/branch-fold-msan.ll
> ===================================================================
> --- test/Transforms/SimplifyCFG/branch-fold-msan.ll
> +++ test/Transforms/SimplifyCFG/branch-fold-msan.ll
> @@ -0,0 +1,29 @@
> +; RUN: opt < %s -simplifycfg -S | FileCheck %s
> +
> +declare void @callee()
> +
> +; Test that conditional branches are not folded with sanitize_memory.
> +define void @caller(i32 %x, i32 %y) sanitize_memory {
> +; CHECK: define void @caller(i32 [[X:%.*]], i32 [[Y:%.*]])
> +; CHECK: icmp slt i32 {{.*}}[[X]]
> +; CHECK: icmp slt i32 {{.*}}[[Y]]
> +; CHECK: ret void
> +
> +entry:
> +  %cmp = icmp slt i32 %x, 0
> +  br i1 %cmp, label %if.then, label %if.end3
> +
> +if.then:                                          ; preds = %entry
> +  %cmp1 = icmp slt i32 %y, 0
> +  br i1 %cmp1, label %if.then2, label %if.end
> +
> +if.then2:                                         ; preds = %if.then
> +  call void @callee()
> +  br label %if.end
> +
> +if.end:                                           ; preds = %if.then2, %if.then
> +  br label %if.end3
> +
> +if.end3:                                          ; preds = %if.end, %entry
> +  ret void
> +}
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>