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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:139
+  // largest legal integer size.
+  unsigned LargestInt = DL.getLargestLegalIntTypeSizeInBits();
+  if (!LargestInt || Size > LargestInt)
----------------
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.


https://reviews.llvm.org/D35035