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

[Matthias Braun via llvm-commits]

Thanks Hans!

> On Aug 21, 2017, at 1:08 PM, Hans Wennborg <hans at chromium.org> wrote:
> 
> 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