[PATCH] D52092: [ValueTracking] Generalize isBytewiseValue into isSplatValue

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: include/llvm/Analysis/ValueTracking.h:229
+  /// truncated to one byte. Otherwise return null.
+  inline Value *isBytewiseValue(Value *V) {
+    return isSplatValue(V, 8);
----------------
jfb wrote:
> bjope wrote:
> > jfb wrote:
> > > bjope wrote:
> > > > jfb wrote:
> > > > > Your target has 16-bit bytes, shouldn't this function (with your change) call `isSplatValue(V, 16)` in your target?
> > > > Correct! We are actually  doing `return isSplatValue(V, DataLayout::getBitsPerByte());` in our llvm fork. But I can't do that in llvm trunk, because there is currently no way to ask for getBitsPerByte().
> > > > 
> > > > Currently we also need to implement/maintain our own version of isSplatValue, because the existing isBytewiseValue function in llvm trunk is not general enough.
> > > Do you have plans to upstream your fork? It's otherwise pretty difficult to semi-support it, and it'll break very easily with changes that are useful to the LLVM community. It sounds like you're trying to incrementally get to a point where you can upstream?
> > I guess it will be hard to upstream full 16-bit bytes unless someone else is interested. There is not backend that supports it in tree, so it would be hard to create test cases. 
> > Our target/backend is for special hardware so that will never go public.
> > 
> > This time I was just trying to reduce some of the diffs we have compared to upstream. The idea was that our more general isSplatValue instead of (or rather as a complement to) isBytewiseValue could be useful to the LLVM community as well.
> > 
> > But this is not uttermost important for us. It is just one out of many functions that we need to override to support 16-bit bytes. We are fully aware of the complexity with maintaining this in our fork. But we do not have that many alternatives. Rebasing to trunk everyday, and running lots of compiler tests keeps your mobile network happy! (that could be a new company slogan...)
> I suggest starting a discussion on LLVM-dev. Changes such as this one don't improve LLVM as-is, and were I to see this code I'd simplify it by removing the indirection. If the LLVM community decides that it wants to support your type of effort then I'm all for incrementally making changes that'll get the support you want. If the community doesn't want to go in that direction (say, because nobody volunteers to do the work and maintain it), then I'm against this type of change because it adds burden without any eventual payoff. We'll just break you in the future it seems!
FWIW, I don't mind removing the indirection. I left it here as a backwards compatibility (in case isBytewiseValue was called from clang or etc).

We happened to have a more general isSplatValue implementation, so the goal was to contribute by replacing isBytewiseValue with our more general isSplatValue. After all, this is a common utility method, so having it specialized on 8-bit values seemed like a limitation. If this patch is a burden for LLVM community (although it did not add much complexity afaict) we can skip it.


Repository:
  rL LLVM

https://reviews.llvm.org/D52092