[SelectionDAG][WebAssembly] Tidy up around endianess and isConstantSplat #68212
Conversation
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-selectiondag ChangesThe BuildVectorSDNode::isConstantSplat function could depend on The intent with this patch is to highlight that endianess doesn't In DAGCombiner the code is slightly refactored. Back in the days when While being at it, tidying up around isConstantSplat, this patch also Full diff: https://github.com/llvm/llvm-project/pull/68212.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index f2bd5137bd20935..944832ed72186d7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -7071,12 +7071,23 @@ SDValue DAGCombiner::visitAND(SDNode *N) {
N1, /*AllowUndef=*/false, /*AllowTruncation=*/true)) {
Constant = C->getAPIntValue();
} else if (BuildVectorSDNode *Vector = dyn_cast<BuildVectorSDNode>(N1)) {
+ unsigned EltBitWidth = Vector->getValueType(0).getScalarSizeInBits();
APInt SplatValue, SplatUndef;
unsigned SplatBitSize;
bool HasAnyUndefs;
+ // Endianess should not matter here. Code below makes sure that we only
+ // use the result if the SplatBitSize is a mulitple of the vector element
+ // size. And after that we AND all element sized parts of the splat
+ // together. So the end result should be the same regardless of in which
+ // order we do those operations.
+ const bool IsBigEndian = false;
bool IsSplat = Vector->isConstantSplat(SplatValue, SplatUndef,
- SplatBitSize, HasAnyUndefs);
- if (IsSplat) {
+ SplatBitSize, HasAnyUndefs,
+ EltBitWidth, IsBigEndian);
+
+ // Make sure that variable 'Constant' is only set if 'SplatBitSize' is a
+ // multiple of 'BitWidth'. Otherwise, we could propagate a wrong value.
+ if (IsSplat && (SplatBitSize % EltBitWidth) == 0) {
// Undef bits can contribute to a possible optimisation if set, so
// set them.
SplatValue |= SplatUndef;
@@ -7085,23 +7096,9 @@ SDValue DAGCombiner::visitAND(SDNode *N) {
// the first vector value and FF for the rest, repeating. We need a mask
// that will apply equally to all members of the vector, so AND all the
// lanes of the constant together.
- unsigned EltBitWidth = Vector->getValueType(0).getScalarSizeInBits();
-
- // If the splat value has been compressed to a bitlength lower
- // than the size of the vector lane, we need to re-expand it to
- // the lane size.
- if (EltBitWidth > SplatBitSize)
- for (SplatValue = SplatValue.zextOrTrunc(EltBitWidth);
- SplatBitSize < EltBitWidth; SplatBitSize = SplatBitSize * 2)
- SplatValue |= SplatValue.shl(SplatBitSize);
-
- // Make sure that variable 'Constant' is only set if 'SplatBitSize' is a
- // multiple of 'BitWidth'. Otherwise, we could propagate a wrong value.
- if ((SplatBitSize % EltBitWidth) == 0) {
- Constant = APInt::getAllOnes(EltBitWidth);
- for (unsigned i = 0, n = (SplatBitSize / EltBitWidth); i < n; ++i)
- Constant &= SplatValue.extractBits(EltBitWidth, i * EltBitWidth);
- }
+ Constant = APInt::getAllOnes(EltBitWidth);
+ for (unsigned i = 0, n = (SplatBitSize / EltBitWidth); i < n; ++i)
+ Constant &= SplatValue.extractBits(EltBitWidth, i * EltBitWidth);
}
}
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 764a873768101c2..bc18d9112d6d0ed 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -161,8 +161,13 @@ bool ISD::isConstantSplatVector(const SDNode *N, APInt &SplatVal) {
unsigned SplatBitSize;
bool HasUndefs;
unsigned EltSize = N->getValueType(0).getVectorElementType().getSizeInBits();
+ // Endianess does not matter here. We are checking for a splat given the
+ // element size of the vector, and if we find such a splat for little endian
+ // layout, then that should be valid also for big endian (as the full vector
+ // size is known to be a multiple of the element size).
+ bool IsBigEndian = false;
return BV->isConstantSplat(SplatVal, SplatUndef, SplatBitSize, HasUndefs,
- EltSize) &&
+ EltSize, IsBigEndian) &&
EltSize == SplatBitSize;
}
@@ -12322,6 +12327,10 @@ bool BuildVectorSDNode::isConstantSplat(APInt &SplatValue, APInt &SplatUndef,
// FIXME: This does not work for vectors with elements less than 8 bits.
while (VecWidth > 8) {
+ // If we can't split in half, stop here.
+ if (VecWidth & 1)
+ break;
+
unsigned HalfSize = VecWidth / 2;
APInt HighValue = SplatValue.extractBits(HalfSize, HalfSize);
APInt LowValue = SplatValue.extractBits(HalfSize, 0);
@@ -12339,6 +12348,12 @@ bool BuildVectorSDNode::isConstantSplat(APInt &SplatValue, APInt &SplatUndef,
VecWidth = HalfSize;
}
+ // FIXME: The loop above only tries to split in halves. But if the input
+ // vector for example is <3 x i16> it wouldn't be able to detect a
+ // SplatBitSize of 16. No idea if that is a design flaw currently limiting
+ // optimizations. I guess that back in the days when this helper was created
+ // vectors normally was power-of-2 sized.
+
SplatBitSize = VecWidth;
return true;
}
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index 61cfcdc914cdb93..70629b2a50a982a 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -2576,6 +2576,8 @@ performVectorTruncZeroCombine(SDNode *N, TargetLowering::DAGCombinerInfo &DCI) {
APInt SplatValue, SplatUndef;
unsigned SplatBitSize;
bool HasAnyUndefs;
+ // Endianness doesn't matter in this context because we are looking for
+ // an all-zero value.
return Splat &&
Splat->isConstantSplat(SplatValue, SplatUndef, SplatBitSize,
HasAnyUndefs) &&
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The BuildVectorSDNode::isConstantSplat function could depend on endianness, and it takes a bool argument that can be used to indicate if big or little endian should be considered when internally casting from a vector to a scalar. However, that argument is default set to false (= little endian). And in many situations, even in target generic code such as DAGCombiner, the endianness isn't specified when using the function. The intent with this patch is to highlight that endianness doesn't matter, depending on the context in which the function is used. In DAGCombiner the code is slightly refactored. Back in the days when the code was written it wasn't possible to request a MinSplatBits size when calling isConstantSplat. Instead the code re-expanded the found SplatValue to match with the EltBitWidth. Now we can just provide EltBitWidth as MinSplatBits and remove the logic for doing the re-expand. While being at it, tidying up around isConstantSplat, this patch also adds an explicit check in BuildVectorSDNode::isConstantSplat to break out from the loop if trying to split an on VecWidth into two halves. Haven't been able to prove that there could be miscompiles involved if not doing so. There are lit tests that trigger that scenario, although I think they happen to later discard the returned SplatValue for other reasons.
|
@bjope LGTM, but it scares me that the isBigEndian argument still defaults to little endian. I think this to minimize changes when it was pulled from x86, but something so generic really shouldn't be making this assumption any more. |
@RKSimon, my first idea was to change so that the argument wouldn't be a default argument any longer. Then I realized that in many situations the endianness does not even matter. For awhile I also played around with adding a special version of the helper for such usage (to make it even more clear when or when endianness should be taken into account). It still got a bit messy, so in the end I went for this smaller step to at least document/cleanup the target agnostic use cases a bit. There are both old/new FIXME:s in isConstantSplat, plus this thing with getting rid of default arguments (and "how to know if endianness matters or not"). In other words, still room for improvements. I can see if I find some time to make another attempt while having this fresh in mind. |
The BuildVectorSDNode::isConstantSplat function could depend on
endianess, and it takes a bool argument that can be used to indicate
if big or little endian should be considered when internally casting
from a vector to a scalar. However, that argument is default set to
false (= little endian). And in many situations, even in target
generic code such as DAGCombiner, the endianess isn't specified when
using the function.
The intent with this patch is to highlight that endianess doesn't
matter, depending on the context in which the function is used.
In DAGCombiner the code is slightly refactored. Back in the days when
the code was written it wasn't possible to request a MinSplatBits
size when calling isConstantSplat. Instead the code re-expanded the
found SplatValue to match with the EltBitWidth. Now we can just
provide EltBitWidth as MinSplatBits and remove the logic for doing
the re-expand.
While being at it, tidying up around isConstantSplat, this patch also
adds an explicit check in BuildVectorSDNode::isConstantSplat to break
out from the loop if trying to split an on VecWidth into two halves.
Haven't been able to prove that there could be miscompiles involved
if not doing so. There are lit tests that trigger that scenario,
although I think they happen to later discard the returned SplatValue
for other reasons.