[PATCH] D27466: [SLP] Fix sign-extends for type-shrinking

Matthew Simpson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Dec 11 13:38:41 PST 2016


mssimpso added a comment.

In https://reviews.llvm.org/D27466#618754, @Gerolf wrote:

> Michael, could you also lend this your expert eye? Do you agree with the extra memory (pair vs vector) and compile-time (look at the loop tree) investment for this fix? Shortening the types may not be worth it at least on some architectures.


Gerolf,

In terms of compile-time, the fix adds a single pass over the *roots* of non-store-rooted trees. The number of roots is equal to the vectorization factor, so this will be minimal. In terms of memory usage, the fix adds a bool for each instruction in the vectorizable trees that we can type-shrink. This also seems minimal to me.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1939
       VecTy = VectorType::get(MinTy, BundleWidth);
-      ExtractCost += TTI->getExtractWithExtendCost(
-          Instruction::SExt, EU.Scalar->getType(), VecTy, EU.Lane);
+      ExtractCost += TTI->getExtractWithExtendCost(ExTy, EU.Scalar->getType(),
+                                                   VecTy, EU.Lane);
----------------
Gerolf wrote:
> I would expect the TTI function to make use of the extra information of the sign bit, but can't see that code. Or is this intentional?
I'm not sure how the TTI function could use sign bit information here. We know that if we have a zext, then the sign bit is zero. If we have a sext, the sign bit can be either one or zero. The TTI function returns the expect cost of the pair of instructions:

```
%0 = extractelement ...
%1 = zext/sext %0 ...
```

We're giving the TTI function the kind of extend we are performing. Is there are case you have in mind that I'm not thinking of?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3521
+  // value is known to be zero.
+  auto isSignBitKnownZero = [&](Value *V) -> bool {
+    bool KnownZero = false;
----------------
Gerolf wrote:
> This looks convoluted. You define a lambda and then use the inverted value. I think what you want i hasSignBitSet[] which returns is KnownOne. Then you use this as IsSigned = hasSignBitSet(). I think in your code you mixup concepts and   intended use.
I'm not sure this is what we want. We're checking if the sign bit is known to be zero, not known to be one. We set IsSigned only if we can't prove the sign bit to be zero (even though in reality it may be). Perhaps the "IsSigned" name is a bit confusing? I'll rename it when rewriting the code below to use "any_of" to address Michael's comment.

To clarify what's happening, we're checking the roots to determine if we know the sign bit is zero. If so, we zext. If we can't prove the sign bit to be zero, we add one to the max bit width and sext.


https://reviews.llvm.org/D27466





More information about the llvm-commits mailing list