[ValueTracking] Generalize isBytewiseValue into isSplatValue

[bjope]

Summary

Added a generalization of isBytewiseValue that is called
isSplatValue. It takes the requested splat size (in bits)
as an additional argument.

The old isBytewiseValue remains in the code as a shortcut
for isSplatValue(V, 8).

This refactoring can be useful both when looking for non
bytewise splats, but also for out-of-tree targets that
for example implement 16-bit bytes (as they now can
override the byte size used in isBytewiseValue more easily).

[bjope]

Might conflict with: https://reviews.llvm.org/D51751

[jfb]

include/llvm/Analysis/ValueTracking.h

229

Your target has 16-bit bytes, shouldn't this function (with your change) call isSplatValue(V, 16) in your target?

[bjope]

include/llvm/Analysis/ValueTracking.h

229

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.

[jfb]

include/llvm/Analysis/ValueTracking.h

229

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?

[bjope]

include/llvm/Analysis/ValueTracking.h

229

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...)

[jfb]

include/llvm/Analysis/ValueTracking.h

229

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!

[bjope]

include/llvm/Analysis/ValueTracking.h

229

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.

[jfb]

include/llvm/Analysis/ValueTracking.h

229

As-is the indirection doesn't seem useful. It might be in a wider context, or if it's part of an incremental refactoring to do something the community wants.

What's more general about your implementation? If it's more general in that it also supports 16-bit bytes then I don't think we want to commit anything until the community decides this is a good idea. So I'm not saying "go away", I'm saying that a discussion should be had on llvm-dev about this.

Burdens are *fine* as long as they're worth it and are maintained.

[bjope]

Not pursuing this. Should have been abandoned a long time ago.

include/llvm/Analysis/ValueTracking.h

class Value;
   ///
   ///      NaN --> true/false (depending on the NaN's sign bit)
   ///       +0 --> true
   ///       -0 --> false
   ///   x > +0 --> true
   ///   x < -0 --> false
   bool SignBitMustBeZero(const Value *V, const TargetLibraryInfo *TLI);
 
-  /// If the specified value can be set by repeating the same byte in memory,
-  /// return the i8 value that it is represented with. This is true for all i8
-  /// values obviously, but is also true for i32 0, i32 -1, i16 0xF0F0, double
-  /// 0.0 etc. If the value can't be handled with a repeated byte store (e.g.
-  /// i16 0x1234), return null.
-  Value *isBytewiseValue(Value *V);
+  /// If the specified value can be set by repeating the same sequence of \p
+  /// SplatSizeInBits bits, return the i<N> value that it is represented by.
+  ///
+  /// If the value can't be represented as a repeated sequence (e.g.
+  /// i16 0x1234 with SplatSizeInBits=8), return null.
+  Value *isSplatValue(Value *V, unsigned SplatSizeInBits);
+
+  /// If the specified value is a bytewise splat value, return the value
+  /// truncated to one byte. Otherwise return null.
+  inline Value *isBytewiseValue(Value *V) {
+    return isSplatValue(V, 8);
+  }
 
   /// Given an aggregrate and an sequence of indices, see if the scalar value
   /// indexed is already around as a register, for example if it were inserted
   /// directly into the aggregrate.
   ///
   /// If InsertBefore is not null, this function will duplicate (modified)
   /// insertvalues when a part of a nested struct is extracted.
   Value *FindInsertedValue(Value *V,

lib/Analysis/ValueTracking.cpp

for (unsigned i = 0; i != NumElts; ++i) {
     auto *CElt = dyn_cast<ConstantFP>(Elt);
     if (!CElt || CElt->isNaN())
       return false;
   }
   // All elements were confirmed not-NaN or undefined.
   return true;
 }
 
-/// If the specified value can be set by repeating the same byte in memory,
-/// return the i8 value that it is represented with.  This is
-/// true for all i8 values obviously, but is also true for i32 0, i32 -1,
-/// i16 0xF0F0, double 0.0 etc.  If the value can't be handled with a repeated
-/// byte store (e.g. i16 0x1234), return null.
-Value *llvm::isBytewiseValue(Value *V) {
-  // All byte-wide stores are splatable, even of arbitrary variables.
-  if (V->getType()->isIntegerTy(8)) return V;
+Value *llvm::isSplatValue(Value *V, unsigned SplatSizeInBits) {
+  // SplatSizeInBits sized integers are splatable
+  if (V->getType()->isIntegerTy(SplatSizeInBits)) return V;
 
   // Handle 'null' ConstantArrayZero etc.
   if (Constant *C = dyn_cast<Constant>(V))
     if (C->isNullValue())
-      return Constant::getNullValue(Type::getInt8Ty(V->getContext()));
+      return Constant::getNullValue(Type::getIntNTy(V->getContext(),
+                                                    SplatSizeInBits));
 
   // Constant float and double values can be handled as integer values if the
-  // corresponding integer value is "byteable".  An important case is 0.0.
+  // corresponding integer value is "splatable".  An important case is 0.0.
   if (ConstantFP *CFP = dyn_cast<ConstantFP>(V)) {
     if (CFP->getType()->isFloatTy())
       V = ConstantExpr::getBitCast(CFP, Type::getInt32Ty(V->getContext()));
     if (CFP->getType()->isDoubleTy())
       V = ConstantExpr::getBitCast(CFP, Type::getInt64Ty(V->getContext()));
     // Don't handle long double formats, which have strange constraints.
   }
 
-  // We can handle constant integers that are multiple of 8 bits.
+  // We can handle constant integers that are multiple of SplatSize bits wide.
   if (ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
-    if (CI->getBitWidth() % 8 == 0) {
-      assert(CI->getBitWidth() > 8 && "8 bits should be handled above!");
+    if (CI->getBitWidth() % SplatSizeInBits == 0) {
+      assert(CI->getBitWidth() > SplatSizeInBits &&
+             "SplatSize bits should be handled above!");
 
-      if (!CI->getValue().isSplat(8))
+      if (!CI->getValue().isSplat(SplatSizeInBits))
         return nullptr;
-      return ConstantInt::get(V->getContext(), CI->getValue().trunc(8));
+      return ConstantInt::get(V->getContext(),
+                              CI->getValue().trunc(SplatSizeInBits));
     }
   }
 
   // A ConstantDataArray/Vector is splatable if all its members are equal and
   // also splatable.
   if (ConstantDataSequential *CA = dyn_cast<ConstantDataSequential>(V)) {
     Value *Elt = CA->getElementAsConstant(0);
-    Value *Val = isBytewiseValue(Elt);
+    Value *Val = isSplatValue(Elt, SplatSizeInBits);
     if (!Val)
       return nullptr;
 
     for (unsigned I = 1, E = CA->getNumElements(); I != E; ++I)
       if (CA->getElementAsConstant(I) != Elt)
         return nullptr;
 
     return Val;