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

[Hans Wennborg via llvm-commits]

r311372. Thanks!

On Mon, Aug 21, 2017 at 12:00 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> Probably worth it. IIRC, this results in miscompiling some old versions of
> mplayer so it has actually been seen in the wild.
>
> +Hans Wennborg
>
> On Mon, Aug 21, 2017 at 11:34 AM Matthias Braun via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> 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
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits