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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/undef-earlyclobber-chain.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple riscv64 -mattr=+v -riscv-enable-subreg-liveness < %s  | FileCheck %s
+; RUN: llc -mtriple riscv64 -mattr=+m,+v -riscv-enable-subreg-liveness < %s  | FileCheck %s
 
----------------
craig.topper wrote:
> paulwalker-arm wrote:
> > craig.topper wrote:
> > > Why do we need to add +m, but have no other changes to the test?
> > 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.
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157872/new/

https://reviews.llvm.org/D157872