[llvm] r239849 - Reapply 239795 - [InstCombine] Propagate non-null facts to call parameters
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 25 18:08:47 PDT 2015
On 08/25/2015 05:08 PM, Pete Cooper wrote:
>
>> On Aug 25, 2015, at 4:54 PM, Philip Reames <listmail at philipreames.com
>> <mailto: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. 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? Where'd we go
wrong? (Is it just the issue below?)
>>
>> 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.
>
> 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 <mailto: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 <mailto: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/3b3d760c/attachment.html>
More information about the llvm-commits
mailing list