[SelectionDAG] Use TypeSize variant of ComputeValueVTs to compute correct offsets for scalable aggregate types.

[paulwalker-arm]

This fixes the issue I reported against D146872.

I investigated making MachinePointerInfo TypeSize aware but that felt wrong because MachinePointerInfo offsets are signed. In the end I've followed the approach used when splitting scalable vector storage ops and used a null MachinePointerInfo.

Whilst not specifically needed today (the LangRef only supports aggregate types that are either all fixed length or all scalable) perhaps the correct answer is for ComputeValueVTs to return StackOffsets, that way we're not a million miles away from supporting aggregate types that mix fixed length and scalable types. StackOffset being signed would then be a better (although not necessarily an easy) fit for MachinePointerInfo.

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/undef-earlyclobber-chain.ll

2 ↗

(On Diff #549926)

Why do we need to add +m, but have no other changes to the test?

[paulwalker-arm]

llvm/test/CodeGen/RISCV/rvv/undef-earlyclobber-chain.ll

2 ↗

(On Diff #549926)

This test triggered a crash that reported the "m" extension is required for the requested operation.

The code quality of vrgatherei16-subreg-liveness.ll was substantially worse after this fix with extra stack accesses and the like. I figured this was vscale mul related and so tried enabling the "m" extension and the original test output was obtained.

I don't know enough to understand why but figured that an existing issue was being masked by the missing "offset is scaled" information.

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/undef-earlyclobber-chain.ll

2 ↗

(On Diff #549926)

I took a look. Looks like we have a t2: i64 = vscale Constant:i64<0> that gets lowered to mul and then immediately converted to a libcall.

I guess we should have a vscale combine for 0?

[craig.topper]

LGTM

[paulwalker-arm]

llvm/test/CodeGen/RISCV/rvv/undef-earlyclobber-chain.ll

2 ↗

(On Diff #549926)

Thanks for the investigation @craig.topper , I can confirm updating getVScale() removes the need for the +m test changes. If there's no objection I'll land it as part of this patch.

[craig.topper]

Should we backport this to LLVM 17?

[paulwalker-arm]

In D157872#4592711, @craig.topper wrote:

Should we backport this to LLVM 17?

I wasn't sure of the relative importance given the original work landed prior to LLVM 16. For AArch64 there's no significant need, I only discovered the bug due to some follow on work that I'm hoping to make public soon.

That said, I've just tried and the patches don't just cherry-pick so if you think this is something you're likely to hit for LLVM 17 just let me know and I'll try the branch route given I guess I'll need to learn how pull request work anyway :)

[paulwalker-arm]

I've created https://github.com/llvm/llvm-project/issues/64768.

[craig.topper]

In D157872#4595092, @paulwalker-arm wrote:

In D157872#4592711, @craig.topper wrote:

Should we backport this to LLVM 17?

I wasn't sure of the relative importance given the original work landed prior to LLVM 16. For AArch64 there's no significant need, I only discovered the bug due to some follow on work that I'm hoping to make public soon.

That said, I've just tried and the patches don't just cherry-pick so if you think this is something you're likely to hit for LLVM 17 just let me know and I'll try the branch route given I guess I'll need to learn how pull request work anyway :)

Wasn't the code added in March 2023? That would have been after LLVM 16 branched.

[paulwalker-arm]

In D157872#4595613, @craig.topper wrote:

In D157872#4595092, @paulwalker-arm wrote:

In D157872#4592711, @craig.topper wrote:

Should we backport this to LLVM 17?

I wasn't sure of the relative importance given the original work landed prior to LLVM 16. For AArch64 there's no significant need, I only discovered the bug due to some follow on work that I'm hoping to make public soon.

That said, I've just tried and the patches don't just cherry-pick so if you think this is something you're likely to hit for LLVM 17 just let me know and I'll try the branch route given I guess I'll need to learn how pull request work anyway :)

Wasn't the code added in March 2023? That would have been after LLVM 16 branched.

You're quite right, I somehow got confused with https://reviews.llvm.org/D94142.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::getCondCode(ISD::CondCode Cond) {
   return SDValue(CondCodeNodes[Cond], 0);
 }
 
 SDValue SelectionDAG::getVScale(const SDLoc &DL, EVT VT, APInt MulImm,
                                 bool ConstantFold) {
   assert(MulImm.getBitWidth() == VT.getSizeInBits() &&
          "APInt size does not match type size!");
 
+  if (MulImm == 0)
+    return getConstant(0, DL, VT);
+
   if (ConstantFold) {
     const MachineFunction &MF = getMachineFunction();
     const Function &F = MF.getFunction();
     ConstantRange CR = getVScaleRange(&F, 64);
     if (const APInt *C = CR.getSingleElement())
       return getConstant(MulImm * C->getZExtValue(), DL, VT);
   }
 

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (const AllocaInst *Alloca = dyn_cast<AllocaInst>(SV)) {
         return visitLoadFromSwiftError(I);
     }
   }
 
   SDValue Ptr = getValue(SV);
 
   Type *Ty = I.getType();
   SmallVector<EVT, 4> ValueVTs, MemVTs;
-  SmallVector<uint64_t, 4> Offsets;
+  SmallVector<TypeSize, 4> Offsets;
   ComputeValueVTs(TLI, DAG.getDataLayout(), Ty, ValueVTs, &MemVTs, &Offsets, 0);
   unsigned NumValues = ValueVTs.size();
   if (NumValues == 0)
     return;
 
   Align Alignment = I.getAlign();
   AAMDNodes AAInfo = I.getAAMetadata();
   const MDNode *Ranges = getRangeMetadata(I);
for (unsigned i = 0; i != NumValues; ++i, ++ChainI) {
     if (ChainI == MaxParallelChains) {
       assert(PendingLoads.empty() && "PendingLoads must be serialized first");
       SDValue Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                                   ArrayRef(Chains.data(), ChainI));
       Root = Chain;
       ChainI = 0;
     }
 
-    SDValue A = DAG.getObjectPtrOffset(dl, Ptr, TypeSize::Fixed(Offsets[i]));
-    SDValue L = DAG.getLoad(MemVTs[i], dl, Root, A,
-                            MachinePointerInfo(SV, Offsets[i]), Alignment,
+    // TODO: MachinePointerInfo only supports a fixed length offset.
+    MachinePointerInfo PtrInfo =
+        !Offsets[i].isScalable() || Offsets[i].isZero()
+            ? MachinePointerInfo(SV, Offsets[i].getKnownMinValue())
+            : MachinePointerInfo();
+
+    SDValue A = DAG.getObjectPtrOffset(dl, Ptr, Offsets[i]);
+    SDValue L = DAG.getLoad(MemVTs[i], dl, Root, A, PtrInfo, Alignment,
                             MMOFlags, AAInfo, Ranges);
     Chains[ChainI] = L.getValue(1);
 
     if (MemVTs[i] != ValueVTs[i])
       L = DAG.getPtrExtOrTrunc(L, dl, ValueVTs[i]);
 
     Values[i] = L;
   }
if (TLI.supportSwiftError()) {
 
     if (const AllocaInst *Alloca = dyn_cast<AllocaInst>(PtrV)) {
       if (Alloca->isSwiftError())
         return visitStoreToSwiftError(I);
     }
   }
 
   SmallVector<EVT, 4> ValueVTs, MemVTs;
-  SmallVector<uint64_t, 4> Offsets;
+  SmallVector<TypeSize, 4> Offsets;
   ComputeValueVTs(DAG.getTargetLoweringInfo(), DAG.getDataLayout(),
                   SrcV->getType(), ValueVTs, &MemVTs, &Offsets, 0);
   unsigned NumValues = ValueVTs.size();
   if (NumValues == 0)
     return;
 
   // Get the lowered operands. Note that we do this after
   // checking if NumResults is zero, because with zero results
for (unsigned i = 0; i != NumValues; ++i, ++ChainI) {
     // See visitLoad comments.
     if (ChainI == MaxParallelChains) {
       SDValue Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                                   ArrayRef(Chains.data(), ChainI));
       Root = Chain;
       ChainI = 0;
     }
 
-    SDValue Add = DAG.getObjectPtrOffset(dl, Ptr, TypeSize::Fixed(Offsets[i]));
+    // TODO: MachinePointerInfo only supports a fixed length offset.
+    MachinePointerInfo PtrInfo =
+        !Offsets[i].isScalable() || Offsets[i].isZero()
+            ? MachinePointerInfo(PtrV, Offsets[i].getKnownMinValue())
+            : MachinePointerInfo();
+
+    SDValue Add = DAG.getObjectPtrOffset(dl, Ptr, Offsets[i]);
     SDValue Val = SDValue(Src.getNode(), Src.getResNo() + i);
     if (MemVTs[i] != ValueVTs[i])
       Val = DAG.getPtrExtOrTrunc(Val, dl, MemVTs[i]);
     SDValue St =
-        DAG.getStore(Root, dl, Val, Add, MachinePointerInfo(PtrV, Offsets[i]),
-                     Alignment, MMOFlags, AAInfo);
+        DAG.getStore(Root, dl, Val, Add, PtrInfo, Alignment, MMOFlags, AAInfo);
     Chains[ChainI] = St;
   }
 
   SDValue StoreNode = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                                   ArrayRef(Chains.data(), ChainI));
   setValue(&I, StoreNode);
   DAG.setRoot(StoreNode);
 }

llvm/test/CodeGen/AArch64/alloca-load-store-scalable-struct.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; RUN: llc < %s | FileCheck %s
 
 target triple = "aarch64-unknown-linux-gnu"
 
 %struct.test = type { <vscale x 2 x double>, <vscale x 2 x double>, <vscale x 2 x double> }
 
 define void @test(ptr %addr) #0 {
 ; CHECK-LABEL: test:
 ; CHECK:       // %bb.0: // %entry
 ; CHECK-NEXT:    str x29, [sp, #-16]! // 8-byte Folded Spill
 ; CHECK-NEXT:    addvl sp, sp, #-3
 ; CHECK-NEXT:    .cfi_escape 0x0f, 0x0c, 0x8f, 0x00, 0x11, 0x10, 0x22, 0x11, 0x18, 0x92, 0x2e, 0x00, 0x1e, 0x22 // sp + 16 + 24 * VG
 ; CHECK-NEXT:    .cfi_offset w29, -16
-; CHECK-NEXT:    mov x8, #2 // =0x2
-; CHECK-NEXT:    mov x9, #4 // =0x4
 ; CHECK-NEXT:    ptrue p0.d
-; CHECK-NEXT:    mov x10, sp
-; CHECK-NEXT:    ld1d { z0.d }, p0/z, [x0, x8, lsl #3]
-; CHECK-NEXT:    ld1d { z1.d }, p0/z, [x0, x9, lsl #3]
+; CHECK-NEXT:    ld1d { z0.d }, p0/z, [x0, #1, mul vl]
+; CHECK-NEXT:    ld1d { z1.d }, p0/z, [x0, #2, mul vl]
 ; CHECK-NEXT:    ld1d { z2.d }, p0/z, [x0]
-; CHECK-NEXT:    st1d { z0.d }, p0, [x10, x8, lsl #3]
-; CHECK-NEXT:    st1d { z1.d }, p0, [x10, x9, lsl #3]
 ; CHECK-NEXT:    st1d { z2.d }, p0, [sp]
+; CHECK-NEXT:    st1d { z1.d }, p0, [sp, #2, mul vl]
+; CHECK-NEXT:    st1d { z0.d }, p0, [sp, #1, mul vl]
 ; CHECK-NEXT:    addvl sp, sp, #3
 ; CHECK-NEXT:    ldr x29, [sp], #16 // 8-byte Folded Reload
 ; CHECK-NEXT:    ret
 entry:
   %ret = alloca %struct.test, align 8
   %val = load %struct.test, ptr %addr
   store %struct.test %val, ptr %ret, align 8
   ret void
 }
 
 attributes #0 = { "target-features"="+sve" }

llvm/test/CodeGen/AArch64/sve-extract-fixed-vector.ll

 ; CHECK:       // %bb.0: // %entry
 ; CHECK-NEXT:    ptrue p0.s
 ; CHECK-NEXT:    ld1w { z0.s }, p0/z, [x0, x1, lsl #2]
 ; CHECK-NEXT:    // kill: def $q0 killed $q0 killed $z0
 ; CHECK-NEXT:    ret
 entry:
   %ptr = getelementptr inbounds i32, i32* %addr, i64 %idx
   %bc = bitcast i32* %ptr to <vscale x 4 x i32>*
-  %ld = load <vscale x 4 x i32>, <vscale x 4 x i32>* %bc, align 16
+  %ld = load volatile <vscale x 4 x i32>, <vscale x 4 x i32>* %bc, align 16
   %out = call <4 x i32> @llvm.vector.extract.v4i32.nxv4i32(<vscale x 4 x i32> %ld, i64 0)
   ret <4 x i32> %out
 }
 
 ;
 ; Extract fixed-width vector from a scalable vector splat.
 ;
 

llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-struct.ll

 ; CHECK-LABEL: test:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    addi sp, sp, -16
 ; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    csrrs a2, vlenb, zero
 ; CHECK-NEXT:    slli a2, a2, 1
 ; CHECK-NEXT:    sub sp, sp, a2
 ; CHECK-NEXT:    .cfi_escape 0x0f, 0x0d, 0x72, 0x00, 0x11, 0x10, 0x22, 0x11, 0x02, 0x92, 0xa2, 0x38, 0x00, 0x1e, 0x22 # sp + 16 + 2 * vlenb
+; CHECK-NEXT:    csrrs a2, vlenb, zero
 ; CHECK-NEXT:    vl1re64.v v8, (a0)
-; CHECK-NEXT:    addi a0, a0, 8
+; CHECK-NEXT:    add a0, a0, a2
 ; CHECK-NEXT:    vl1re64.v v9, (a0)
 ; CHECK-NEXT:    addi a0, sp, 16
 ; CHECK-NEXT:    vs1r.v v8, (a0)
-; CHECK-NEXT:    addi a2, sp, 24
+; CHECK-NEXT:    add a2, a0, a2
 ; CHECK-NEXT:    vs1r.v v9, (a2)
 ; CHECK-NEXT:    vl1re64.v v8, (a2)
 ; CHECK-NEXT:    vl1re64.v v9, (a0)
 ; CHECK-NEXT:    vsetvli zero, a1, e64, m1, ta, ma
 ; CHECK-NEXT:    vfadd.vv v8, v9, v8
 ; CHECK-NEXT:    csrrs a0, vlenb, zero
 ; CHECK-NEXT:    slli a0, a0, 1
 ; CHECK-NEXT:    add sp, sp, a0