[PATCH] D153311: [Attributor] Unify AAMemoryLocation and AAMemoryBehavior

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1997
+                                  bool &UsedAssumedInformation,
+                                  bool RecurseForSelectAndPHI = true);
 
----------------
This was accidentally included. Will remove it. Ignore for now.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1379
+    }
+  }
   return true;
----------------
This was accidentally included. Will remove it. Ignore for now.


================
Comment at: llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll:37
 ; TUNIT-LABEL: define {{[^@]+}}@test
-; TUNIT-SAME: (ptr nocapture nofree readonly [[X:%.*]]) #[[ATTR0]] {
+; TUNIT-SAME: (ptr nocapture nofree readnone [[X:%.*]]) #[[ATTR1:[0-9]+]] {
 ; TUNIT-NEXT:  entry:
----------------
This is a curious case. I think, the result is sound in some sense, but not as the lang ref is Witten.

`%X` is read (`%tmp2` in TUNIT below, or in the original call to make the `byval` copy), but the result is unused.
The original `byval` argument `%X` is only written by the callee, which makes it `readnone` on this side of the call, conceptually.

All that said, I need to check this in detail and avoid it. Lang ref should say violating read none results in poison, but it says it results in UB.


================
Comment at: llvm/test/Transforms/Attributor/ArgumentPromotion/inalloca.ll:11
 define internal i32 @f(ptr inalloca(%struct.ss) %s) {
-; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: read)
+; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: readwrite)
 ; CHECK-LABEL: define {{[^@]+}}@f
----------------
I think we are more conservative wrt. inalloca. Which is fine with me, given that I do not understand it.


================
Comment at: llvm/test/Transforms/Attributor/ArgumentPromotion/variadic.ll:34
+; CGSCC-NEXT:    tail call void (ptr, ptr, ptr, ptr, ptr, ...) @callee_t0f(ptr readnone undef, ptr readnone undef, ptr readnone undef, ptr readnone undef, ptr readnone undef, ptr noundef nonnull readnone byval([[STRUCT_TT0:%.*]]) align 8 dereferenceable(16) @t45)
+; CGSCC-NEXT:    ret i32 0
 ;
----------------
I am not sure if the byval should be a forced read. I guess that might solve the problem above as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153311/new/

https://reviews.llvm.org/D153311