[PATCH] D35035: [InstCombine] Prevent memcpy generation for small data size

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:139
+  // largest legal integer size.
+  unsigned LargestInt = DL.getLargestLegalIntTypeSizeInBits();
+  if (!LargestInt || Size > LargestInt)
----------------
spatel wrote:
> hiraditya wrote:
> > jfb wrote:
> > > Why does this make sense? I don't understand why we'd want to tie this to the largest legal integer type, and not have it be its own target parameter.
> > We used 'largest legal integer size' because that will fit in a register for sure. I think making a target specific parameter seems reasonable, or maybe using TargetLowering.h:getMaxStoresPerMemcpy() which is already available.
> I'm not understanding this discussion...
> This patch is trying to apply some target-based constraint to a questionable (reverse) IR canonicalization that currently just pulls a number out of the air.
> The ultimate goal would be to simply always canonicalize to memcpy and not expand it ever in instcombine as mentioned in D52081.
> But we don't do that (yet) because we're afraid of missed optimizations that can't be replicated in the backend.
> Expanding memcpy for performance using target parameters belongs in a late, target-aware pass (and as mentioned, it already exists), not early in generic instcombine.
Let me try to expand a bit, and let me know if that makes sense:

IMO `LargestLegalIntTypeSizeInBits` doesn't make sense to use here. On ARM I might want to use paired integer load / store for memcpy, or paired Q registers. These have nothing to do with the largest legal integer type. It doesn't matter what the patch is trying to do or what is currently being done: the patch is adding something weird. It shouldn't.


https://reviews.llvm.org/D35035