[PowerPC] Fix wrong ElemSIze when calling isConsecutiveLS()

[ZhangKang]

Summary

This issue from the bugzilla: https://bugs.llvm.org/show_bug.cgi?id=41177

When the two operands for BUILD_VECTOR are same, we will get assert error.

llvm::SDValue combineBVOfConsecutiveLoads(llvm::SDNode*, llvm::SelectionDAG&): Assertion `!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) && "The loads cannot be both consecutive and reverse consecutive."' failed.

This error caused by the wrong ElemSIze when calling isConsecutiveLS(). We should use getScalarType().getStoreSize(); to get the ElemSize instread of getScalarSizeInBits() / 8.

[jsji]

LGTM. Thanks for reducing the testcase and fixing!

[efriedma]

This fix is wrong. For vectors where the element type is not byte-sized, loads can't be "consecutive" in the sense of this function; the store size of <2 x i1> is one byte.

The right fix is to return early from combineBVOfConsecutiveLoads for vector types where getVectorElementType().isByteSized() is false. Or you could probably just check !isTypeLegal(N->getValueType(0)); even if the transform is legal, it likely isn't profitable for illegal vector types.

[jsji]

In D60811#1472300, @efriedma wrote:

For vectors where the element type is not byte-sized, loads can't be "consecutive" in the sense of this function; the store size of <2 x i1> is one byte.

I think isConcecitiveLS will early return false too with this fix. getScalarSizeInBits() / 8 != Bytes.

[efriedma]

I guess that's right, technically; "getScalarSizeInBits() / 8" will round down, and getStoreSize() will round up. But still, please fix the code so it's clear that check is happening.

[jsji]

In D60811#1472361, @efriedma wrote:

please fix the code so it's clear that check is happening.

Sure, we will follow up with a NFC patch with your suggestions. Thanks for pointing out.

[ZhangKang]

In D60811#1472361, @efriedma wrote:

I guess that's right, technically; "getScalarSizeInBits() / 8" will round down, and getStoreSize() will round up. But still, please fix the code so it's clear that check is happening.

I have submitted a NFC patch D61076 to follow your suggestion.

llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp

 /// to reorder the vector.
 static SDValue combineBVOfConsecutiveLoads(SDNode *N, SelectionDAG &DAG) {
   assert(N->getOpcode() == ISD::BUILD_VECTOR &&
          "Should be called with a BUILD_VECTOR node");
 
   SDLoc dl(N);
   bool InputsAreConsecutiveLoads = true;
   bool InputsAreReverseConsecutive = true;
-  unsigned ElemSize = N->getValueType(0).getScalarSizeInBits() / 8;
+  unsigned ElemSize = N->getValueType(0).getScalarType().getStoreSize();
   SDValue FirstInput = N->getOperand(0);
   bool IsRoundOfExtLoad = false;
 
   if (FirstInput.getOpcode() == ISD::FP_ROUND &&
       FirstInput.getOperand(0).getOpcode() == ISD::LOAD) {
     LoadSDNode *LD = dyn_cast<LoadSDNode>(FirstInput.getOperand(0));
     IsRoundOfExtLoad = LD->getExtensionType() == ISD::EXTLOAD;
   }

llvm/trunk/test/CodeGen/PowerPC/pr41177.ll

+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64le-unknown-linux-gnu < %s 
+; REQUIRES: asserts
+
+define protected swiftcc void @"$s22LanguageServerProtocol13HoverResponseV8contents5rangeAcA13MarkupContentV_SnyAA8PositionVGSgtcfC"() {
+  %1 = load <2 x i64>, <2 x i64>* undef, align 16
+  %2 = load i1, i1* undef, align 8
+  %3 = insertelement <2 x i1> undef, i1 %2, i32 0
+  %4 = shufflevector <2 x i1> %3, <2 x i1> undef, <2 x i32> zeroinitializer
+  %5 = select <2 x i1> %4, <2 x i64> zeroinitializer, <2 x i64> %1
+  store <2 x i64> %5, <2 x i64>* undef, align 8
+  ret void
+}