[X86] Make EltsFromConsecutiveLoads code path work only on vectors it expects

[mkuper]

Summary

EltsFromConsecutiveLoads was apparently only ever called for 128-bit vectors, and assumed this implicitly.
r223518 started calling it for AVX-sized vectors, causing the code path that had this assumption to crash.

This makes the assumption explicit by adding a check.

[spatel]

Hi Michael,
Thanks for notifying me about this bug. While your patch will prevent the crash, it will also prevent vector loads for i8 / i16, right? Is it possible to fix the condition to avoid the crash while still generating a vector load for all data types?

For reference - while stepping through this path, I noticed another bug:
http://llvm.org/bugs/show_bug.cgi?id=21790

For your testcase with i64, it looks like we'll end up with the optimal codegen, but only because we'll crack the 256-bit vector in half and then the two i64 loads will be treated like a full vector load the next time we call EltsFromConsecutiveLoads().

Can you add your testcase to test/CodeGen/X86/vec_loadsingles.ll (name change is probably in order at this point), so we have all of the related testcases in one file? See inline comments for the testcase itself.

test/CodeGen/X86/elts-from-loads-256.ll

1

Use FileCheck for expected output.

7–9

Remove unnecessary test elements: addrspace, attributes.

23

Add newline

[mkuper]

Thanks for the review, Sanjay.

Right about the test-case, it was bugpoint-reduced, and I didn't clean it up enough, thanks for pointing it out.
Will fix and put it in the right place.

Regarding vector loads of i8 / i16 - I believe that didn't work to begin with.
The existing code fires only when NumElems == 4, and creates a ResNode of type v2i64. So for the bitcast in the end to be valid, the element size must be 32 bit. Or did I miss something?

Regarding the PR - you're right, it looks like unneeded domain crossing. I'd rather resolve the crash first, though.

[mkuper]

Test-case updated.

[spatel]

In D6579#5, @mkuper wrote:

Regarding vector loads of i8 / i16 - I believe that didn't work to begin with.
The existing code fires only when NumElems == 4, and creates a ResNode of type v2i64. So for the bitcast in the end to be valid, the element size must be 32 bit. Or did I miss something?

Ah, ok. Please add a 'TODO' comment there that we should handle other types and/or file a PR. I don't think there's any reason to limit the optimization to just 32-bit elements.

[mkuper]

Closed by commit rL223922 (authored by @mkuper).

lib/Target/X86/X86ISelLowering.cpp

if (LDBase->hasAnyUseOfValue(1)) {
                                      SDValue(NewLd.getNode(), 1));
       DAG.ReplaceAllUsesOfValueWith(SDValue(LDBase, 1), NewChain);
       DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(LDBase, 1),
                              SDValue(NewLd.getNode(), 1));
     }
 
     return NewLd;
   }
-  if (NumElems == 4 && LastLoadedElt == 1 &&
+  if (NumElems == 4 && LastLoadedElt == 1 && (EltVT.getSizeInBits() == 32) &&
       DAG.getTargetLoweringInfo().isTypeLegal(MVT::v2i64)) {
     SDVTList Tys = DAG.getVTList(MVT::v2i64, MVT::Other);
     SDValue Ops[] = { LDBase->getChain(), LDBase->getBasePtr() };
     SDValue ResNode =
         DAG.getMemIntrinsicNode(X86ISD::VZEXT_LOAD, DL, Tys, Ops, MVT::i64,
                                 LDBase->getPointerInfo(),
                                 LDBase->getAlignment(),
                                 false/*isVolatile*/, true/*ReadMem*/,

test/CodeGen/X86/elts-from-loads-256.ll

+; RUN: llc -mcpu=corei7-avx < %s
+; EltsFromConsecutiveLoads has a code-path that should not kick-in for AVX-sized vectors
+; Make sure there is no crash
+target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-win32"
+
+; Function Attrs: nounwind
+define void @test() #0 {
+  %1 = load i64 addrspace(1)** undef, align 8
+  %2 = getelementptr inbounds i64 addrspace(1)* %1, i64 0
+  %3 = load i64 addrspace(1)* %2
+  %4 = insertelement <4 x i64> undef, i64 %3, i32 0
+  %5 = load i64 addrspace(1)** undef, align 8
+  %6 = getelementptr inbounds i64 addrspace(1)* %5, i64 1
+  %7 = load i64 addrspace(1)* %6
+  %8 = insertelement <4 x i64> %4, i64 %7, i32 1
+  %9 = shufflevector <4 x i64> %8, <4 x i64> undef, <4 x i32> <i32 0, i32 1, i32 4, i32 5>
+  store <4 x i64> %9, <4 x i64>* undef
+  ret void
+}
+
+attributes #0 = { nounwind }
 No newline at end of file