[PATCH] D150967: [MemCpyOpt] precommit test for memcpy removal for immutable arguments from attributes (NFC)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:398
+declare void @fnr(ptr noalias readonly)
+define noundef i32 @noalias_readonly_i8(ptr align 4 noalias readonly %val) {
+; CHECK-LABEL: @noalias_readonly_i8(
----------------
khei4 wrote:
> nikic wrote:
> > Drop noundef, it's not relevant. Can also change return type to void. I also don't think `noalias readonly` on the function argument matter, only `align 4`. The `noalias readonly` are relevant on the call only.
> > Drop noundef, it's not relevant. Can also change return type to void. 
> 
> Thanks! They seemed correct!
> 
> > I also don't think noalias readonly on the function argument matter, only align 4. The noalias readonly are relevant on the call only.
> 
> I worry about the case like
> 
> ```
> #include <thread>
> 
> void ro(int &d) { d; }
> 
> void w(int &d) { d = 13; }
> 
> int main() {
>   int *src, *dest1, *dest2;
>   *src = 42;
>   std::memcpy(dest1, src, sizeof dest1);
>   std::thread ro_t1(ro, dest1);
>   std::memcpy(dest2, src, sizeof dest2);
>   std::thread w_t(w, dest2);
>   std::thread ro_t2(ro, dest2);
> }
> ```
> (although I'm not sure `noalias` can be actually attributed for this specific case)
> 
> It seems like the transformation would break the `noalias` property of the callee parameter, because memcpy might be used to introduce the actual `noalias` property.  
Yes, I think you're right. Basically, in terms of clobbers, the things we have to ensure are:

* The memcpy dst is not modified by the call. (The noalias readonly ensures this.)
* The memcpy dst is not modified between the memcpy and the call. (Needs MSSA clobber check.)
* The memcpy src is not modified between the memcpy and the call. (Needs MSSA clobber check.)
* The memcpy src is not modified by the call. Needs a ModRef check. This is the bit that could be ensured by noalias readonly the function parameter. (But also through other means, e.g. if the source is an unescaped alloca.)


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:409
+  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %val1, ptr align 4 %val, i64 1, i1 false)
+  %0 = call noundef i32 @fnr(ptr align 4 noalias readonly %val1)
+  ret i32 %0
----------------
khei4 wrote:
> khei4 wrote:
> > nikic wrote:
> > > Also needs `nocapture`, otherwise the address may be significant.
> > Thanks! Yeah, removing memcpy by only `noalias` and `readonly` might broke capturing behavior! 
> > Also I noticed `readnone` doesn't need `nocapture`, so I'll add a test for that case :)
> > 
> > Also I noticed readnone doesn't need nocapture, so I'll add a test for that case :)
> 
> Sorry, that's not. Maybe `readnone` only guarantee no-deref, so capturing with `readnone` seems valid. 
Yes, a readnone argument can still be captured. Even if the function itself is `memory(none)` it can still be captured via return value or via unwind/divergence side effects.


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

https://reviews.llvm.org/D150967