[Attributor] Do not manifest noundef for dead positions

[okura]

Summary

Even if noundef is deduced for a position, we should not manifest it when the position is dead.
This is because the associated values with dead positions are replaced with undef values by AAIsDead.

[jdoerfert]

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

7814

I'm not sure this should not be a false.

[sstefan1]

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Maybe this can be tested if we actually address the TODO for AANoUndef in AAUB? I think with that, this can be caught in module pass?

Just a suggestion.

[jdoerfert]

In D86565#2237295, @sstefan1 wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Maybe this can be tested if we actually address the TODO for AANoUndef in AAUB? I think with that, this can be caught in module pass?

Just a suggestion.

Right. That should work.

[okura]

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.
Some tests are affected by this change.

[okura]

• change CheckBBLivenessOnly to true
• add affected tests
• add new tests

[jdoerfert]

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

[okura]

In D86565#2237383, @jdoerfert wrote:

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

I have difficulty in doing that because segfault happens when a fixpoint is indicated in initialize and updateImpl is not called even once.

[jdoerfert]

In D86565#2237391, @okura wrote:

In D86565#2237383, @jdoerfert wrote:

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

I have difficulty in doing that because segfault happens when a fixpoint is indicated in initialize and updateImpl is not called even once.

TBH, I think we need to remove the assertion that you see (it should not be a segfault). And replace it with code that causes a pessimistic fixpoint for all attributes created after we are done with the update stage. They can initialize but that is it.

[okura]

In D86565#2237393, @jdoerfert wrote:

In D86565#2237391, @okura wrote:

In D86565#2237383, @jdoerfert wrote:

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

I have difficulty in doing that because segfault happens when a fixpoint is indicated in initialize and updateImpl is not called even once.

TBH, I think we need to remove the assertion that you see (it should not be a segfault). And replace it with code that causes a pessimistic fixpoint for all attributes created after we are done with the update stage. They can initialize but that is it.

Yes, it was assertion failure. I used a wrong term, sorry for the confusion.
I agree to remove the assertion. Can I work on that?

[sstefan1]

Are you going to move the AAIsDead query to the update?

[okura]

In D86565#2238813, @sstefan1 wrote:

Are you going to move the AAIsDead query to the update?

No, I'm going to allow to query (initialize) new AA in the manifestation stage. That will save us from querying AAIsDead before the manifestation stage.

[okura]

In D86565#2238954, @okura wrote:

In D86565#2238813, @sstefan1 wrote:

Are you going to move the AAIsDead query to the update?

No, I'm going to allow to query (initialize) new AA in the manifestation stage. That will save us from querying AAIsDead before the manifestation stage.

I have uploaded a new patch for allowing to query AA in manifest . D86635

[okura]

Delete AAIsDead query before manifestation because other patches (D86635, D86734) make it possible to query new AAs in manifest.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

if (isa<CastInst>(*I) || isa<GetElementPtrInst>(*I))
       TrackUse = true;
     return TrackUse;
   }
 
   /// See AbstractAttribute::getAsStr().
   const std::string getAsStr() const override {
     return getAssumed() ? "noundef" : "may-undef-or-poison";
   }
+
+  ChangeStatus manifest(Attributor &A) override {
+    // We don't manifest noundef attribute for dead positions because the
+    // associated values with dead positions would be replaced with undef
+    // values.
+    if (A.isAssumedDead(getIRPosition(), nullptr, nullptr))
+      return ChangeStatus::UNCHANGED;
+    return AANoUndef::manifest(A);
+  }
 };
 
 struct AANoUndefFloating : public AANoUndefImpl {
   AANoUndefFloating(const IRPosition &IRP, Attributor &A)
       : AANoUndefImpl(IRP, A) {}
 
   /// See AbstractAttribute::initialize(...).
   void initialize(Attributor &A) override {

llvm/test/Transforms/Attributor/ArgumentPromotion/inalloca.ll

entry:
   %r = call i32 @f(%struct.ss* inalloca %S)
   ret i32 %r
 }
 
 ; Argpromote can't promote %a because of the icmp use.
 define internal i1 @g(%struct.ss* %a, %struct.ss* inalloca %b) nounwind  {
 ; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@g
-; IS__CGSCC____-SAME: (%struct.ss* nocapture nofree noundef nonnull readnone align 4 dereferenceable(8) [[A:%.*]], %struct.ss* inalloca nocapture nofree noundef nonnull writeonly align 4 dereferenceable(8) [[B:%.*]])
+; IS__CGSCC____-SAME: (%struct.ss* nocapture nofree nonnull readnone align 4 dereferenceable(8) [[A:%.*]], %struct.ss* inalloca nocapture nofree nonnull writeonly align 4 dereferenceable(8) [[B:%.*]])
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    ret i1 undef
 ;
 entry:
   %c = icmp eq %struct.ss* %a, %b
   ret i1 %c
 }
 

llvm/test/Transforms/Attributor/IPConstantProp/pthreads.ll

 ; FIXME: nocapture & noalias for %alloc2 in %call3
 
 define dso_local i32 @main() {
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@main()
 ; IS__TUNIT____-NEXT:  entry:
 ; IS__TUNIT____-NEXT:    [[ALLOC1:%.*]] = alloca i8, align 8
 ; IS__TUNIT____-NEXT:    [[ALLOC2:%.*]] = alloca i8, align 8
 ; IS__TUNIT____-NEXT:    [[THREAD:%.*]] = alloca i64, align 8
-; IS__TUNIT____-NEXT:    [[CALL:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @foo, i8* noalias nocapture nofree noundef readnone align 536870912 undef)
-; IS__TUNIT____-NEXT:    [[CALL1:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @bar, i8* noalias nofree noundef nonnull readnone align 8 dereferenceable(8) "no-capture-maybe-returned" undef)
+; IS__TUNIT____-NEXT:    [[CALL:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @foo, i8* noalias nocapture nofree readnone align 536870912 undef)
+; IS__TUNIT____-NEXT:    [[CALL1:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @bar, i8* noalias nofree nonnull readnone align 8 dereferenceable(8) "no-capture-maybe-returned" undef)
 ; IS__TUNIT____-NEXT:    [[CALL2:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @baz, i8* noalias nocapture nofree noundef nonnull readnone align 8 dereferenceable(1) [[ALLOC1]])
 ; IS__TUNIT____-NEXT:    [[CALL3:%.*]] = call i32 @pthread_create(i64* noundef nonnull align 8 dereferenceable(8) [[THREAD]], %union.pthread_attr_t* noalias nocapture noundef align 536870912 null, i8* (i8*)* noundef nonnull @buz, i8* noalias nofree noundef nonnull readnone align 8 dereferenceable(1) "no-capture-maybe-returned" [[ALLOC2]])
 ; IS__TUNIT____-NEXT:    ret i32 0
 ;
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@main()
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    [[ALLOC1:%.*]] = alloca i8, align 8
 ; IS__CGSCC____-NEXT:    [[ALLOC2:%.*]] = alloca i8, align 8
entry:
   ret i32 0
 }
 
 declare !callback !0 dso_local i32 @pthread_create(i64*, %union.pthread_attr_t*, i8* (i8*)*, i8*)
 
 define internal i8* @foo(i8* %arg) {
 ; IS__TUNIT____: Function Attrs: nofree nosync nounwind readnone willreturn
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@foo
-; IS__TUNIT____-SAME: (i8* noalias nofree noundef readnone returned align 536870912 "no-capture-maybe-returned" [[ARG:%.*]])
+; IS__TUNIT____-SAME: (i8* noalias nofree readnone returned align 536870912 "no-capture-maybe-returned" [[ARG:%.*]])
 ; IS__TUNIT____-NEXT:  entry:
 ; IS__TUNIT____-NEXT:    ret i8* null
 ;
 ; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@foo
-; IS__CGSCC____-SAME: (i8* noalias nofree noundef readnone returned align 536870912 "no-capture-maybe-returned" [[ARG:%.*]])
+; IS__CGSCC____-SAME: (i8* noalias nofree readnone returned align 536870912 "no-capture-maybe-returned" [[ARG:%.*]])
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    ret i8* null
 ;
 entry:
   ret i8* %arg
 }
 
 define internal i8* @bar(i8* %arg) {
 ; IS__TUNIT____: Function Attrs: nofree nosync nounwind readnone willreturn
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@bar
-; IS__TUNIT____-SAME: (i8* noalias nofree noundef nonnull readnone returned align 8 dereferenceable(8) "no-capture-maybe-returned" [[ARG:%.*]])
+; IS__TUNIT____-SAME: (i8* noalias nofree nonnull readnone returned align 8 dereferenceable(8) "no-capture-maybe-returned" [[ARG:%.*]])
 ; IS__TUNIT____-NEXT:  entry:
 ; IS__TUNIT____-NEXT:    ret i8* bitcast (i8** @GlobalVPtr to i8*)
 ;
 ; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readnone willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@bar
-; IS__CGSCC____-SAME: (i8* nofree noundef readnone returned "no-capture-maybe-returned" [[ARG:%.*]])
+; IS__CGSCC____-SAME: (i8* nofree readnone returned "no-capture-maybe-returned" [[ARG:%.*]])
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    ret i8* bitcast (i8** @GlobalVPtr to i8*)
 ;
 entry:
   ret i8* %arg
 }
 
 define internal i8* @baz(i8* %arg) {

llvm/test/Transforms/Attributor/IPConstantProp/thread_local_acs.ll

 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 @gtl = dso_local thread_local global i32 0, align 4
 @gsh = dso_local global i32 0, align 4
 
 define internal i32 @callee(i32* %thread_local_ptr, i32* %shared_ptr) {
 ; IS__TUNIT____: Function Attrs: nofree nosync nounwind readonly willreturn
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@callee
-; IS__TUNIT____-SAME: (i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[THREAD_LOCAL_PTR:%.*]], i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[SHARED_PTR:%.*]])
+; IS__TUNIT____-SAME: (i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[THREAD_LOCAL_PTR:%.*]], i32* nocapture nofree nonnull readonly align 4 dereferenceable(4) [[SHARED_PTR:%.*]])
 ; IS__TUNIT____-NEXT:  entry:
 ; IS__TUNIT____-NEXT:    [[TMP:%.*]] = load i32, i32* [[THREAD_LOCAL_PTR]], align 4
 ; IS__TUNIT____-NEXT:    [[TMP1:%.*]] = load i32, i32* @gsh, align 4
 ; IS__TUNIT____-NEXT:    [[ADD:%.*]] = add nsw i32 [[TMP]], [[TMP1]]
 ; IS__TUNIT____-NEXT:    ret i32 [[ADD]]
 ;
 ; IS__CGSCC____: Function Attrs: nofree norecurse nosync nounwind readonly willreturn
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@callee
-; IS__CGSCC____-SAME: (i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[THREAD_LOCAL_PTR:%.*]], i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[SHARED_PTR:%.*]])
+; IS__CGSCC____-SAME: (i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) [[THREAD_LOCAL_PTR:%.*]], i32* nocapture nofree nonnull readonly align 4 dereferenceable(4) [[SHARED_PTR:%.*]])
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    [[TMP:%.*]] = load i32, i32* [[THREAD_LOCAL_PTR]], align 4
 ; IS__CGSCC____-NEXT:    [[TMP1:%.*]] = load i32, i32* @gsh, align 4
 ; IS__CGSCC____-NEXT:    [[ADD:%.*]] = add nsw i32 [[TMP]], [[TMP1]]
 ; IS__CGSCC____-NEXT:    ret i32 [[ADD]]
 ;
 entry:
   %tmp = load i32, i32* %thread_local_ptr, align 4
   %tmp1 = load i32, i32* %shared_ptr, align 4
   %add = add nsw i32 %tmp, %tmp1
   ret i32 %add
 }
 
 define dso_local void @caller() {
 ; IS__TUNIT____-LABEL: define {{[^@]+}}@caller()
 ; IS__TUNIT____-NEXT:  entry:
-; IS__TUNIT____-NEXT:    call void @broker(i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) @gtl, i32 (i32*, i32*)* noundef nonnull @callee, i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) undef)
+; IS__TUNIT____-NEXT:    call void @broker(i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) @gtl, i32 (i32*, i32*)* noundef nonnull @callee, i32* nocapture nofree nonnull readonly align 4 dereferenceable(4) undef)
 ; IS__TUNIT____-NEXT:    ret void
 ;
 ; IS__CGSCC____-LABEL: define {{[^@]+}}@caller()
 ; IS__CGSCC____-NEXT:  entry:
 ; IS__CGSCC____-NEXT:    call void @broker(i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) @gtl, i32 (i32*, i32*)* noundef nonnull @callee, i32* nocapture nofree noundef nonnull readonly align 4 dereferenceable(4) @gsh)
 ; IS__CGSCC____-NEXT:    ret void
 ;
 entry:
   call void @broker(i32* nonnull @gtl, i32 (i32*, i32*)* nonnull @callee, i32* nonnull @gsh)
   ret void
 }
 
 declare !callback !0 dso_local void @broker(i32*, i32 (i32*, i32*)*, i32*)
 
 !1 = !{i64 1, i64 0, i64 2, i1 false}
 !0 = !{!1}

llvm/test/Transforms/Attributor/misc_crash.ll

define internal i32* @func1a([1 x i32]* %arg) {
   %ptr = getelementptr inbounds [1 x i32], [1 x i32]* %arg, i64 0, i64 0
   ret i32* %ptr
 }
 ; UTC_ARGS: --enable
 
 define internal void @func2a(i32* %0) {
 ; CHECK: Function Attrs: nofree nosync nounwind willreturn writeonly
 ; CHECK-LABEL: define {{[^@]+}}@func2a
-; CHECK-SAME: (i32* nocapture nofree noundef nonnull writeonly align 4 dereferenceable(4) [[TMP0:%.*]])
+; CHECK-SAME: (i32* nocapture nofree nonnull writeonly align 4 dereferenceable(4) [[TMP0:%.*]])
 ; CHECK-NEXT:    store i32 0, i32* @var2, align 4
 ; CHECK-NEXT:    ret void
 ;
   store i32 0, i32* %0
   ret void
 }
 
 define i32 @func2() {

llvm/test/Transforms/Attributor/noundef.ll

 ; CHECK-NEXT:    call void @bar(i32* noundef nonnull align 4 dereferenceable(4) [[X]])
 ; CHECK-NEXT:    ret void
 ;
   %x = alloca i32
   call void @unknown()
   call void @bar(i32* %x)
   ret void
 }
+
+define internal i8* @returned_dead() {
+; CHECK-LABEL: define internal noalias align 536870912 i8* @returned_dead(
+; CHECK-NEXT:    call void @unknown()
+; CHECK-NEXT:    ret i8* undef
+;
+  call void @unknown()
+  ret i8* null
+}
+
+define void @caller1() {
+; CHECK-LABEL: @caller1(
+; CHECK-NEXT:    [[TMP1:%.*]] = call i8* @returned_dead()
+; CHECK-NEXT:    ret void
+;
+  call i8* @returned_dead()
+  ret void
+}
+
+define internal void @argument_dead_callback_callee(i8* %c) {
+; CHECK-LABEL: @argument_dead_callback_callee(
+; CHECK-NEXT:    call void @unknown()
+; CHECK-NEXT:    ret void
+;
+  call void @unknown()
+  ret void
+}
+
+define void @callback_caller() {
+; IS__TUNIT____-LABEL: @callback_caller(
+; IS__TUNIT____-NEXT:    call void @callback_broker(void (i8*)* noundef @argument_dead_callback_callee, i8* noalias nocapture nofree readnone align 536870912 undef)
+; IS__TUNIT____-NEXT:    ret void
+;
+; IS__CGSCC____-LABEL: @callback_caller(
+; IS__CGSCC____-NEXT:    call void @callback_broker(void (i8*)* noundef @argument_dead_callback_callee, i8* noalias nocapture nofree noundef readnone align 536870912 null)
+; IS__CGSCC____-NEXT:    ret void
+;
+  call void @callback_broker(void (i8*)* @argument_dead_callback_callee, i8* null)
+  ret void
+}
+
+declare !callback !0 void @callback_broker(void (i8*)*, i8*)
+!1 = !{i64 0, i64 1, i1 false}
+!0 = !{!1}