[DAGCombiner] Fix wrong folding of AND dag nodes.

[andreadb]

Summary

This patch fixes the logic in the DAGCombiner that folds an AND node according to rule:

(and (X (load V)), C) -> (X (load V))

An AND between a vector load 'X' and a constant build_vector 'C' can be folded into the load itself only if we can prove that the AND operation is redundant.
The algorithm implemented by 'visitAND' firstly computes the splat value 'S' from C, and then checks if S has the lower 'B' bits set (where B is the size in bits of the vector element type). The algorithm takes into account also the 'undef' bits in the splat mask.

Unfortunately, the algorithm only works under the assumption that the sizeof S is a multiple of the vector element type. This assumption is not always valid. For example:

%load = load <3 x i8>, <3 x i8>* %ptr
%and = <3 x i8> %load, <i8 undef, i8 undef, i8 95>

Here, the splat value returned by method 'BuildVectorSDNode::isConstantSplat' is APInt(12b, 1520u, 1520s).
The undef bits of the splat mask are APInt(12b, 15u, 15s).
The unified splat plus undef mask 'SU' is ApInt(12b, 1535u, 1535s).
The size in bits of the vector element type is 8. However, the splat value has a size of 12.

Before this patch, the algorithm would have wrongly truncated SU from 12 to 8 bits. So, the resulting splat value was incorrectly propagated as constant 255. Since 255 is a mask with all-bits set, the AND was considered redundant and therefore wrongly removed.

With this patch, we conservatively avoid folding the AND if the splat bits are not compatible with the vector element type.

Added X86 test and-load-fold.ll

Please let me know if ok to submit.

[mkuper]

LGTM, thanks for fixing this.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2813 ↗

(On Diff #21291)

It's a bit of a shame there's no early exit here, but I don't see how that can be done without outlining the entire "fold (and (X (load ([non_ext|any_ext|zero_ext] V))), c) -> (X (load ([non_ext|zero_ext] V)))" combine into a separate function, so I guess we'll have to live with it.

[andreadb]

Thanks Michael!
Committed revision 231563.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (const ConstantSDNode *C = dyn_cast<ConstantSDNode>(N1)) {
         // than the size of the vector lane, we need to re-expand it to
         // the lane size.
         if (BitWidth > SplatBitSize)
           for (SplatValue = SplatValue.zextOrTrunc(BitWidth);
                SplatBitSize < BitWidth;
                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 % BitWidth == 0) {
           Constant = APInt::getAllOnesValue(BitWidth);
           for (unsigned i = 0, n = SplatBitSize/BitWidth; i < n; ++i)
             Constant &= SplatValue.lshr(i*BitWidth).zextOrTrunc(BitWidth);
         }
       }
+    }
 
     // If we want to change an EXTLOAD to a ZEXTLOAD, ensure a ZEXTLOAD is
     // actually legal and isn't going to get expanded, else this is a false
     // optimisation.
     bool CanZextLoadProfitably = TLI.isLoadExtLegal(ISD::ZEXTLOAD,
                                                     Load->getValueType(0),
                                                     Load->getMemoryVT());
 

llvm/trunk/test/CodeGen/X86/and-load-fold.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=generic < %s | FileCheck %s
+
+; Verify that the DAGCombiner doesn't wrongly remove the 'and' from the dag.
+
+define i8 @foo(<4 x i8>* %V) {
+; CHECK-LABEL: foo:
+; CHECK: pand
+; CHECK: ret
+entry:
+  %Vp = bitcast <4 x i8>* %V to <3 x i8>*
+  %V3i8 = load <3 x i8>, <3 x i8>* %Vp, align 4
+  %0 = and <3 x i8> %V3i8, <i8 undef, i8 undef, i8 95>
+  %1 = extractelement <3 x i8> %0, i64 2
+  ret i8 %1
+}