[PATCH] D63604: [Attributor] Deduce "nonnull" attribute

[Johannes Doerfert via Phabricator via llvm-commits]

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