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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka marked an inline comment as done.
vitalybuka added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3182
   if (isa<UndefValue>(V))
-    return UndefInt8;
+    return;
 
----------------
jfb wrote:
> eugenis wrote:
> > vitalybuka wrote:
> > > jfb wrote:
> > > > Should `Undef` count as `Other`? Or have its own count?
> > > Undef means we don't care what it's there, so we can fill we any pattern.
> > > Other counts bytes which are different from most frequent value and will have to be fixed, e.g. after memset
> > Should we count undefs separately in the histogram? It seems like it would not add any complexity or runtime cost, and the caller can just add the two number if that's what they need.
> Right, it seems like we might want the following information:
> 
> * Value is one of 256 bytes were tracking.
> * Value wasn't something we looked into, for X bytes.
> * Value was `undef`.
> * Value was padding (same as `undef`).
> 
> We can figure out some of this, but not all, from the current patch. Is it useful to know the rest? I'm not sure! I haven't inspected all uses to be able to convince myself.
Right now I only use "count" and "other". Future users can easily count and return undefs and update interface if needed. But I would prefer to do so when needed. Also maybe at some point it would be useful to return entire histogram.

But I am OK to include undef if you believe it could be used soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64111