[PATCH] D63604: [Attributor] Deduce "nonnull" attribute
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 13:37:05 PDT 2019
jdoerfert added a comment.
define internal i32* @f1(i32* %arg) {
bb:
%tmp = icmp eq i32* %arg, null
br i1 %tmp, label %bb9, label %bb1
bb1: ; preds = %bb
%tmp2 = load i32, i32* %arg, align 4
%tmp3 = icmp eq i32 %tmp2, 0
br i1 %tmp3, label %bb6, label %bb4
bb4: ; preds = %bb1
%tmp5 = getelementptr inbounds i32, i32* %arg, i64 1
%tmp5b = tail call i32* @f3(i32* %tmp5)
br label %bb9
bb6: ; preds = %bb1
%tmp7 = tail call i32* @f2(i32* %arg)
ret i32* %tmp7
bb9: ; preds = %bb4, %bb
%tmp10 = phi i32* [ %tmp5, %bb4 ], [ inttoptr (i64 4 to i32*), %bb ]
ret i32* %tmp10
}
define internal i32* @f2(i32* %arg) {
bb:
%tmp = tail call i32* @f1(i32* %arg)
ret i32* %tmp
}
define dso_local noalias i32* @f3(i32* %arg) {
bb:
%tmp = call i32* @f1(i32* %arg)
ret i32* null
}
Another test case to show some shortcomings of the current approach.
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:835
+ return ImmutableCallSite(&getAnchoredValue()).getArgument(ArgNo);
+ }
+
----------------
jdoerfert wrote:
> You can pass this value to the `AbstractAttribute` constructor (via a new `AANonNullImpl` and `AANonNull` constructor so there is no need to overload this method and keep track of the argument number.
It seems without the argument number you have to do a linear search. I didn't realize that. Can we store the argument number but still pass the `CS.getArgOperand(ArgNo)` to the `AANonNullImpl` constructor as well?
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:854
+ Value &V = *getAssociatedValue();
+
+ auto *AANonNull = A.getAAFor<AANonNullImpl>(*this, V);
----------------
uenoku wrote:
> jdoerfert wrote:
> > uenoku wrote:
> > > jdoerfert wrote:
> > > > `isKnowNonNull(&V, ...)` ?
> > > >`isKnowNonNull(&V, ...)`?
> > >
> > > Could you explain more detail?
> > >
> > >
> > >
> > >
> > Sorry. Any reason you do not use the `isKnownNonNull` functionality here (as you did above)? The check for an `AANonNullImpl` attribute will only succeed if the values is a call or argument.
> I understand you are saying about `isKnownNonZero` :).
That is what I meant all along ;)
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:676
+
+ /// See AANonNull::isAssumedNonNull().
+ virtual const std::string getAsStr() const override {
----------------
Copy & Paste
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:801
+ if (!NonNullCSArgAA || !NonNullCSArgAA->isAssumedNonNull())
+ return false;
+
----------------
We have to make sure we do not get the `AANonNullImpl` for the argument back when none exists for the call site argument. So you have to test that the associated value of `NonNullCSArgAA` is the call site operand, or you check that `NonNullCSArgAA` is not `this`.
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:838
+
+ChangeStatus AANonNullCallSiteArgument::updateImpl(Attributor &A) {
+ Value &V = *getAssociatedValue();
----------------
Add a strongly worded comment here telling people they should **not** (explicitly) look at the argument of the callee in this method.
================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:843
+ indicateOptimisticFixpoint();
+ return ChangeStatus::CHANGED;
+ }
----------------
It's only about the assumed state. So, `UNCHANGED` as we assumed non-null already otherwise updateImpl wouldn't be called.
================
Comment at: llvm/test/Transforms/FunctionAttrs/nonnull.ll:147
+define internal void @test13(i8* %a) {
+; ATTRIBUTOR: define internal void @test13(i8* nonnull %a)
+ tail call void @test13_helper(i8* %a)
----------------
We derive `nonnull` here because `test13` is dead, correct? Maybe that is not what you wanted to test ;)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63604/new/
https://reviews.llvm.org/D63604
More information about the llvm-commits
mailing list