[PATCH] D55120: [MemCpyOpt] memset->memcpy forwarding with undef tail

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:173
+      Loc = MemoryLocation::getForArgument(II, 0, TLI);
+      return ModRefInfo::Mod;
     default:
----------------
nikic wrote:
> efriedma wrote:
> > This probably makes sense, but it looks like it'll impact other transforms; would it be possible to test separately?  Or does it actually not have any impact on other transforms for some reason?
> > 
> > Looks fine otherwise.
> It seems that it indeed does not impact other transforms, at least not in any way that I can see. The main consumer of `GetLocation()` is `getDependency()`, for which:
> 
>  * GVN only deals with loads.
>  * DSE has it's own mechanism for determining location.
>  * MemCpyOpt does not call getDependency on memsets (prior to this change).
> 
> The other consumer is `getCallSiteDependencyFrom()`, where previously memset would have been treated as a normal call-site. However in both cases (known location or call-site) the result will ultimately be determinded by AA.
> 
> ---
> 
> What I'm unsure about is whether this is correct wrt handling of volatile. I see in the code above that an ordered (and non-monotonic) StoreInst will return ModRef with empty location. Maybe memset needs the same treatment for volatile?
We don't really use volatile memsets anywhere important, but yes, probably best to be conservative and treat them as ModRef.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55120