[llvm] r311229 - [Inliner] Fix a nasty bug when inlining a non-recursive trace of

[Matthias Braun via llvm-commits]

Would this be worthy of llvm-5.0 or is it too late/risky/uncommon a case?

- Matthias

> On Aug 18, 2017, at 11:56 PM, Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Author: chandlerc
> Date: Fri Aug 18 23:56:11 2017
> New Revision: 311229
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=311229&view=rev
> Log:
> [Inliner] Fix a nasty bug when inlining a non-recursive trace of
> a function into itself.
> 
> We tried to fix this before in r306495 but that got reverted as the
> assert was actually hit.
> 
> This fixes the original bug (which we seem to have lost track of with
> the revert) by blocking a second remapping when the function being
> inlined is also the caller and the remapping could succeed but
> erroneously.
> 
> The included test case would actually load from an inlined copy of the
> alloca before this change, failing to load the stored value and
> miscompiling.
> 
> Many thanks to Richard Smith for diagnosing a user miscompile to this
> bug, and to Kyle for the first attempt and initial analysis and David Li
> for remembering the issue and how to fix it and suggesting the patch.
> I'm just stitching it together and landing it. =]
> 
> Modified:
>    llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
>    llvm/trunk/test/Transforms/Inline/recursive.ll
> 
> Modified: llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp?rev=311229&r1=311228&r2=311229&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp Fri Aug 18 23:56:11 2017
> @@ -341,8 +341,9 @@ void PruningFunctionCloner::CloneBlock(c
>               SimplifyInstruction(NewInst, BB->getModule()->getDataLayout())) {
>         // On the off-chance that this simplifies to an instruction in the old
>         // function, map it back into the new function.
> -        if (Value *MappedV = VMap.lookup(V))
> -          V = MappedV;
> +        if (NewFunc != OldFunc)
> +          if (Value *MappedV = VMap.lookup(V))
> +            V = MappedV;
> 
>         if (!NewInst->mayHaveSideEffects()) {
>           VMap[&*II] = V;
> 
> Modified: llvm/trunk/test/Transforms/Inline/recursive.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/recursive.ll?rev=311229&r1=311228&r2=311229&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/Inline/recursive.ll (original)
> +++ llvm/trunk/test/Transforms/Inline/recursive.ll Fri Aug 18 23:56:11 2017
> @@ -40,3 +40,34 @@ declare void @bar(i8* %in)
> 
> declare i32 @foo(i32 %param)
> 
> +; Check that when inlining a non-recursive path into a function's own body that
> +; we get the re-mapping of instructions correct.
> +define i32 @test_recursive_inlining_remapping(i1 %init, i8* %addr) {
> +; CHECK-LABEL: define i32 @test_recursive_inlining_remapping(
> +bb:
> +  %n = alloca i32
> +  br i1 %init, label %store, label %load
> +; CHECK-NOT:     alloca
> +;
> +; CHECK:         %[[N:.*]] = alloca i32
> +; CHECK-NEXT:    br i1 %init,
> +
> +store:
> +  store i32 0, i32* %n
> +  %cast = bitcast i32* %n to i8*
> +  %v = call i32 @test_recursive_inlining_remapping(i1 false, i8* %cast)
> +  ret i32 %v
> +; CHECK-NOT:     call
> +;
> +; CHECK:         store i32 0, i32* %[[N]]
> +; CHECK-NEXT:    %[[CAST:.*]] = bitcast i32* %[[N]] to i8*
> +; CHECK-NEXT:    %[[INLINED_LOAD:.*]] = load i32, i32* %[[N]]
> +; CHECK-NEXT:    ret i32 %[[INLINED_LOAD]]
> +;
> +; CHECK-NOT:     call
> +
> +load:
> +  %castback = bitcast i8* %addr to i32*
> +  %n.load = load i32, i32* %castback
> +  ret i32 %n.load
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits