[PATCH] D37880: Fix an out-of-bounds shufflevector index bug

[Jatin Bhateja via Phabricator via llvm-commits]

jbhateja added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14472
 
-    NearestPow2 = PowerOf2Ceil(MaxIndex);
+    NearestPow2 = PowerOf2Ceil(MaxIndex + 1);
     if (InVT.isSimple() && (NearestPow2 > 2) &&
----------------
george.burgess.iv wrote:
> jbhateja wrote:
> > If MaxIndex is same as MaxSize of vector it will cause later split (line #14475) to be futile.
> I tried adding `InVT.getVectorNumElements() != NearestPow2` into the conditional below, and it seems to substantially change the output of some test cases (e.g. one of the tests in test/CodeGen/X86/avx512-shuffles/partial_permute.ll goes from 4 vector instructions to 18).
> 
> Since I don't know this code, and it looks like your comment applies to this code even without this patch, I'd rather not have that change as a part of this.
Hi George, 

Your fix is fine, I looked at it again but for the boundary case it will be more appropriate to AND another condition for if block which is  MaxIndex < NearestPow2. This shall save unnecessary splif of vector.

So if condition should be 

    if (InVT.isSimple() && (NearestPow2 > 2) &&
        ( MaxIndex < NearestPow2) &&
        ((NumElems * 2) < NearestPow2)) {

Thanks


https://reviews.llvm.org/D37880