[llvm] r239849 - Reapply 239795 - [InstCombine] Propagate non-null facts to call parameters
Pete Cooper via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 25 21:10:41 PDT 2015
Sent from my iPhone
> On Aug 25, 2015, at 6:08 PM, Philip Reames <listmail at philipreames.com> wrote:
>
>> On 08/25/2015 05:08 PM, Pete Cooper wrote:
>>
>>> On Aug 25, 2015, at 4:54 PM, Philip Reames <listmail at philipreames.com> wrote:
>>>
>>> Pete,
>>>
>>> I'm not really following why your example is problematic, even for an non-zero address space. We're not changing anything about the interpretation of the null pointer with this change. We're simply propagating that the pointer *isn't* null, whatever meaning that might have for the given address space. What I am missing in your explanation? Where does this go wrong?
>> Ah, sorry i should have explained better.
>>
>> What i was describing was a bit of a what-if. If we had an optimization which propagated the not null to the function itself, which isn’t difficult now that you’ve added it to all the eligible call sites, then it would be possible to incorrectly remove code in the example I gave.
> I'm willing to accept the non-null propagation from the call sites to the callee for an internal function.
Yeah. I'm still trying to work out the best way to do that. We don't seem to have a good pass that fits. But that's an issue for later I guess.
> However, I'm still not seeing why the transform is incorrect, If we proved the pointer can't be null at the one and only call site, the check inside the function obviously can't ever see null. What's wrong with this picture?
I think it was distracting of me to give an example where not null might go wrong. This example shows that if we incorrectly put not null on the pointer then the kind of optimisation that might incorrectly trigger.
> Where'd we go wrong? (Is it just the issue below?)
It is just the issue below yeah.
>>>
>>> Are you possibly trying to say that we false concluded it was nonnull at the call site? i.e. Did you find a bug in isKnownNonNull for non-zero address space pointers?
>> But yes, this is the real bug. This is the relevant code, which really needs to be checking for address spaces.
>>
>> // Global values are not null unless extern weak.
>> if (const GlobalValue *GV = dyn_cast<GlobalValue>(V))
>> return !GV->hasExternalWeakLinkage();
> Want to send a patch? I'll be happy to review.
Cool. Thanks. I'll try get something done tomorrow.
Cheers
Pete
>>
>> Cheers,
>> Pete
>>>
>>> Philip
>>>
>>>> On 08/25/2015 11:09 AM, Pete Cooper wrote:
>>>> Hey Philip
>>>>
>>>> We just came across a case internally where this optimization applied not null to a pointer with an address space other than 0.
>>>>
>>>> Sorry, I reviewed this and really should have thought about that at the time.
>>>>
>>>> What are your thoughts on this? I believe we can only assume nullness on address space 0 and have to leave all others alone.
>>>>
>>>> I don’t think we have an optimization pipeline which will expose this, but lets assume that we have something like:
>>>>
>>>> foo(&g)
>>>> ...
>>>> foo(int *addresspace(1) p) {
>>>> if (p != null) { bar(); }
>>>> }
>>>>
>>>> then you could imagine us propagating the not null from the call to the callee (if all calls to the callee have not null), and so we have
>>>>
>>>> foo(&g __attribute__((not null)) )
>>>> ...
>>>> foo(int *addresspace(1) __attribute__((not null)) p) {
>>>> if (p != null) { bar(); }
>>>> }
>>>>
>>>> and then some optimization would constant fold the if to give us
>>>>
>>>> foo(int *addresspace(1) __attribute__((not null)) p) {
>>>> bar();
>>>> }
>>>>
>>>> which might be incorrect if 0 was a valid address on address space 1.
>>>>
>>>> Cheers,
>>>> Pete
>>>>> On Jun 16, 2015, at 1:24 PM, Philip Reames <listmail at philipreames.com> wrote:
>>>>>
>>>>> Author: reames
>>>>> Date: Tue Jun 16 15:24:25 2015
>>>>> New Revision: 239849
>>>>>
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D239849-26view-3Drev&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=zCZOTfrNGX-URsmw7bhhRVxYQvKoRUQU_sSrbdhbtX8&e= Log:
>>>>> Reapply 239795 - [InstCombine] Propagate non-null facts to call parameters
>>>>>
>>>>> The original change broke clang side tests. I will be submitting those momentarily. This change includes post commit feedback on the original change from from Pete Cooper.
>>>>>
>>>>> Original Submission comments:
>>>>> If a parameter to a function is known non-null, use the existing parameter attributes to record that fact at the call site. This has no optimization benefit by itself - that I know of - but is an enabling change for https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9129&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=xdQ7r5usnFj24cbkbi7xrqULBEj9KxRiv5IHualA0vA&e= .
>>>>>
>>>>> Differential Revision: https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9132&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=_0McocxwRnNqXY2DzhWVljrDylGa8auvq2rGWSwe85c&e=
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/include/llvm/IR/CallSite.h
>>>>> llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>>>> llvm/trunk/test/CodeGen/NVPTX/intrin-nocapture.ll
>>>>> llvm/trunk/test/Transforms/Inline/byval-tail-call.ll
>>>>> llvm/trunk/test/Transforms/InstCombine/select.ll
>>>>>
>>>>> Modified: llvm/trunk/include/llvm/IR/CallSite.h
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_include_llvm_IR_CallSite.h-3Frev-3D239849-26r1-3D239848-26r2-3D239849-26view-3Ddiff&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=CVzzdRdB5oRpo9UQEYi2KTvToOeg6iF0XMftoXK5DFo&e= ==============================================================================
>>>>> --- llvm/trunk/include/llvm/IR/CallSite.h (original)
>>>>> +++ llvm/trunk/include/llvm/IR/CallSite.h Tue Jun 16 15:24:25 2015
>>>>> @@ -27,6 +27,7 @@
>>>>> #define LLVM_IR_CALLSITE_H
>>>>>
>>>>> #include "llvm/ADT/PointerIntPair.h"
>>>>> +#include "llvm/ADT/iterator_range.h"
>>>>> #include "llvm/IR/Attributes.h"
>>>>> #include "llvm/IR/CallingConv.h"
>>>>> #include "llvm/IR/Instructions.h"
>>>>> @@ -150,6 +151,9 @@ public:
>>>>> }
>>>>>
>>>>> IterTy arg_end() const { return (*this)->op_end() - getArgumentEndOffset(); }
>>>>> + iterator_range<IterTy> args() const {
>>>>> + return iterator_range<IterTy>(arg_begin(), arg_end());
>>>>> + }
>>>>> bool arg_empty() const { return arg_end() == arg_begin(); }
>>>>> unsigned arg_size() const { return unsigned(arg_end() - arg_begin()); }
>>>>>
>>>>>
>>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_InstCombine_InstCombineCalls.cpp-3Frev-3D239849-26r1-3D239848-26r2-3D239849-26view-3Ddiff&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=BsxlpsGfx6RU9emQe7mFuMYbqKdxo8JJtB7UqpwfeKU&e= ==============================================================================
>>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
>>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue Jun 16 15:24:25 2015
>>>>> @@ -1391,11 +1391,29 @@ static IntrinsicInst *FindInitTrampoline
>>>>> // visitCallSite - Improvements for call and invoke instructions.
>>>>> //
>>>>> Instruction *InstCombiner::visitCallSite(CallSite CS) {
>>>>> +
>>>>> if (isAllocLikeFn(CS.getInstruction(), TLI))
>>>>> return visitAllocSite(*CS.getInstruction());
>>>>>
>>>>> bool Changed = false;
>>>>>
>>>>> + // Mark any parameters that are known to be non-null with the nonnull
>>>>> + // attribute. This is helpful for inlining calls to functions with null
>>>>> + // checks on their arguments.
>>>>> + unsigned ArgNo = 0;
>>>>> + for (Value *V : CS.args()) {
>>>>> + if (!CS.paramHasAttr(ArgNo+1, Attribute::NonNull) &&
>>>>> + isKnownNonNull(V)) {
>>>>> + AttributeSet AS = CS.getAttributes();
>>>>> + AS = AS.addAttribute(CS.getInstruction()->getContext(), ArgNo+1,
>>>>> + Attribute::NonNull);
>>>>> + CS.setAttributes(AS);
>>>>> + Changed = true;
>>>>> + }
>>>>> + ArgNo++;
>>>>> + }
>>>>> + assert(ArgNo == CS.arg_size() && "sanity check");
>>>>> +
>>>>> // If the callee is a pointer to a function, attempt to move any casts to the
>>>>> // arguments of the call/invoke.
>>>>> Value *Callee = CS.getCalledValue();
>>>>>
>>>>> Modified: llvm/trunk/test/CodeGen/NVPTX/intrin-nocapture.ll
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_NVPTX_intrin-2Dnocapture.ll-3Frev-3D239849-26r1-3D239848-26r2-3D239849-26view-3Ddiff&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=kckzUpGGcdQiQaHhAEGRU6W6UAfgOgvxHHo3Wf-D9_4&e= ==============================================================================
>>>>> --- llvm/trunk/test/CodeGen/NVPTX/intrin-nocapture.ll (original)
>>>>> +++ llvm/trunk/test/CodeGen/NVPTX/intrin-nocapture.ll Tue Jun 16 15:24:25 2015
>>>>> @@ -11,7 +11,7 @@ declare i32 addrspace(1)* @llvm.nvvm.ptr
>>>>> ; CHECK: @bar
>>>>> define void @bar() {
>>>>> %t1 = alloca i32
>>>>> -; CHECK: call i32 addrspace(1)* @llvm.nvvm.ptr.gen.to.global.p1i32.p0i32(i32* %t1)
>>>>> +; CHECK: call i32 addrspace(1)* @llvm.nvvm.ptr.gen.to.global.p1i32.p0i32(i32* nonnull %t1)
>>>>> ; CHECK-NEXT: store i32 10, i32* %t1
>>>>> %t2 = call i32 addrspace(1)* @llvm.nvvm.ptr.gen.to.global.p1i32.p0i32(i32* %t1)
>>>>> store i32 10, i32* %t1
>>>>>
>>>>> Modified: llvm/trunk/test/Transforms/Inline/byval-tail-call.ll
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_Inline_byval-2Dtail-2Dcall.ll-3Frev-3D239849-26r1-3D239848-26r2-3D239849-26view-3Ddiff&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=jdGpDmu_-liQ4QSf2MasxZbLc3jcwqygDUOa4Yzaxh4&e= ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/Inline/byval-tail-call.ll (original)
>>>>> +++ llvm/trunk/test/Transforms/Inline/byval-tail-call.ll Tue Jun 16 15:24:25 2015
>>>>> @@ -33,7 +33,7 @@ define void @frob(i32* %x) {
>>>>> ; CHECK: %[[POS:.*]] = alloca i32
>>>>> ; CHECK: %[[VAL:.*]] = load i32, i32* %x
>>>>> ; CHECK: store i32 %[[VAL]], i32* %[[POS]]
>>>>> -; CHECK: {{^ *}}call void @ext(i32* %[[POS]]
>>>>> +; CHECK: {{^ *}}call void @ext(i32* nonnull %[[POS]]
>>>>> ; CHECK: tail call void @ext(i32* null)
>>>>> ; CHECK: ret void
>>>>> tail call void @qux(i32* byval %x)
>>>>>
>>>>> Modified: llvm/trunk/test/Transforms/InstCombine/select.ll
>>>>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_InstCombine_select.ll-3Frev-3D239849-26r1-3D239848-26r2-3D239849-26view-3Ddiff&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=pW5HrUeNAokTQHaGtkCTws1Ku_LNanlrzInZl5Ykkdo&e= ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/InstCombine/select.ll (original)
>>>>> +++ llvm/trunk/test/Transforms/InstCombine/select.ll Tue Jun 16 15:24:25 2015
>>>>> @@ -1265,7 +1265,7 @@ define i32 @test77(i1 %flag, i32* %x) {
>>>>> ; load does.
>>>>> ; CHECK-LABEL: @test77(
>>>>> ; CHECK: %[[A:.*]] = alloca i32, align 1
>>>>> -; CHECK: call void @scribble_on_i32(i32* %[[A]])
>>>>> +; CHECK: call void @scribble_on_i32(i32* nonnull %[[A]])
>>>>> ; CHECK: store i32 0, i32* %x
>>>>> ; CHECK: %[[P:.*]] = select i1 %flag, i32* %[[A]], i32* %x
>>>>> ; CHECK: load i32, i32* %[[P]]
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.cs.uiuc.edu_mailman_listinfo_llvm-2Dcommits&d=BQIDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=1F4F5ne45cMxjcevazqUxYm1cFJzKct3OPthe_IDLv8&s=5hlvP4QWrrzH4QOwn4guDATXRPO64TYAQrR4ljD7zYI&e=
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150825/f2092935/attachment.html>
More information about the llvm-commits
mailing list