DAG: Use getStoreSize when expanding dynamic vector indexing

[arsenm]

Summary

I'm not sure why the FIXME was there, since the ABI size of the vector
shouldn't matter for the element offset.

Fixes bug 42304.

[tlively]

That was simple! Thank you!

[bjope]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

5573

Aren't vectors (of non-bytesized elements) packed when stored to memory?

I know that there is code that assumes a packed layout, but also code that does not assume a packed layout. So I don't think that LLVM is consistent.

Most of the time my impression is that vector elements with irregular sizes isn't handled well (when it comes to loads/stores/bitcast) and vectorizers try to avoid creating such vectors (in combination with load/store/bitcast). Without having looked at the details here, I figure that the extractelement is lowered into doing the extract by first storing the vector to memory and then doing a read using the dynamic index. So basically extract/insert element with a dynamic index results in store of the vector, so such operations does not work well for irregular types either.

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

5573

Any vector with elements < 8-bits that touches memory is broken. I don't think there's a consistent interpretation between targets, although I think there's a general desire to move towards treating them as packed

[bjope]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

5573

So the removed assert, that kind of said that this piece of code only works correctly for byte-sized elements, should probably still be there (asserting that "EltVT.getSizeInBits() == EltVT.getStoreSizeInBits()" ).

[arsenm]

I'm not sure this is actually correct

[arsenm]

I'm not bothering with SelectionDAG improvements anymore

lib/CodeGen/SelectionDAG/TargetLowering.cpp

SDValue TargetLowering::getVectorElementPointer(SelectionDAG &DAG,
                                                 SDValue Index) const {
   SDLoc dl(Index);
   // Make sure the index type is big enough to compute in.
   Index = DAG.getZExtOrTrunc(Index, dl, VecPtr.getValueType());
 
   EVT EltVT = VecVT.getVectorElementType();
 
   // Calculate the element offset and add it to the pointer.
-  unsigned EltSize = EltVT.getSizeInBits() / 8; // FIXME: should be ABI size.
-  assert(EltSize * 8 == EltVT.getSizeInBits() &&
-         "Converting bits to bytes lost precision");
+  unsigned EltSize = EltVT.getStoreSize();
 
   Index = clampDynamicVectorIndex(DAG, Index, VecVT, dl);
 
   EVT IdxVT = Index.getValueType();
 
   Index = DAG.getNode(ISD::MUL, dl, IdxVT, Index,
                       DAG.getConstant(EltSize, dl, IdxVT));
   return DAG.getNode(ISD::ADD, dl, IdxVT, VecPtr, Index);

test/CodeGen/WebAssembly/bug42304.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=wasm32-unknown-unknown < %s | FileCheck %s
+
+define void @dynamic_vector_extract_nonpow2() {
+; CHECK-LABEL: dynamic_vector_extract_nonpow2:
+; CHECK:         .functype dynamic_vector_extract_nonpow2 () -> ()
+; CHECK-NEXT:    .local i32, i64
+; CHECK-NEXT:  # %bb.0: # %bb
+; CHECK-NEXT:    global.get __stack_pointer
+; CHECK-NEXT:    i32.const 16
+; CHECK-NEXT:    i32.sub
+; CHECK-NEXT:    local.tee 0
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    i64.store8 10
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    i64.store16 8
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    i64.store 0
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    i32.load16_u 0
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    i32.and
+; CHECK-NEXT:    i32.const 6
+; CHECK-NEXT:    i32.mul
+; CHECK-NEXT:    i32.or
+; CHECK-NEXT:    local.tee 0
+; CHECK-NEXT:    i64.load32_u 0
+; CHECK-NEXT:    local.tee 1
+; CHECK-NEXT:    i64.store32 0
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    i32.const 4
+; CHECK-NEXT:    i32.add
+; CHECK-NEXT:    i64.load16_u 0
+; CHECK-NEXT:    i64.const 32
+; CHECK-NEXT:    i64.shl
+; CHECK-NEXT:    i64.or
+; CHECK-NEXT:    i64.const 4398046511103
+; CHECK-NEXT:    i64.and
+; CHECK-NEXT:    i64.const 32
+; CHECK-NEXT:    i64.shr_u
+; CHECK-NEXT:    i64.store16 0
+; CHECK-NEXT:    # fallthrough-return-void
+; CHECK-NEXT:    end_function
+bb:
+  %L = load i16, i16* undef
+  %E = extractelement <2 x i42> zeroinitializer, i16 %L
+  store i42 %E, i42* undef
+  ret void
+}