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

[Kohei Asano via Phabricator via llvm-commits]

khei4 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(
----------------
nikic wrote:
> 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.)
Thank you for the review! It saves me a lot!
My example was a little subtle to say the necessity for `noalias`, but now become clear!


================
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
----------------
nikic wrote:
> 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.
Thanks! I see!


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

https://reviews.llvm.org/D150967