[PATCH] D64111: Add getMostFrequentByte and use for isBytewiseValue implementation

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3182
   if (isa<UndefValue>(V))
-    return UndefInt8;
+    return;
 
----------------
Should `Undef` count as `Other`? Or have its own count?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3206
+  if (C->isNullValue()) {
+    H.Values[0] += Size;
+    return;
----------------
vitalybuka wrote:
> bjope wrote:
> > Not sure that we are guaranteed to get Size number of zeroes here. Not unless we also add a check for `DL.typeSizeEqualsStoreSize(V->getType)`. Consider if we for example have an i12, then only 12 bits out of 16 is known to be zero.
> > 
> > (this might be a bug on trunk as well, or maybe I've missed some condition somewhere regarding how this is used)
> > 
> > 
> > 
> yes, it's not precise, as not precise the ConstantFP handling below and isSplat(8) handling in ConstantInt
> I guess it does not change behavior of IsBytewiseValueTest, and it's good enough for my future patch.
> 
> Also I think this is the best behavior for current users. If we memset zero there then we should be good. Same does not work below for  (CI->getBitWidth() % 8) and for non zero initiazer, we had to give up and count it against Other.
> 
> If we count non aligned isNullValue against "Other", it will change behavior of IsBytewiseValueTest. 
I think it's worth a comment on over-estimate, but it seems fine as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64111