[RISCV] Add a test showing incorrect codegen

[frasercrmck]

Summary

This patch adds a test which shows how the compiler incorrectly sets the
size and alignment of a stack object used to indirectly pass vector
types to functions.

In the particular example, the test passes a <4 x i8> vector type to a
function and creates a stack object of size and alignment equal to 4
bytes. However, the code generated to set up that parameter has been
scalarized and stores each element as individual XLEN-sized values. Thus
on RV32 this stores 16 bytes and on RV64 32 bytes, both of which clobber
the stack. Similarly, the alignment is set up as the alignment
of the vector type, which is not necessarily the natural alignment of XLEN.

[frasercrmck]

I posted something about this bug late last year but haven't found the time to progress with it. So I thought I'd at least put up a test case so it's tracked.

[luismarques]

Are we moving away from update_llc_test_checks.py for vector tests?

[jrtc27]

In D95025#2521409, @luismarques wrote:

Are we moving away from update_llc_test_checks.py for vector tests?

Please no, update_llc_test_checks.py is one of the best things to have happened to LLVM tests, both as an upstream developer and for significant downstream forks.

[jrtc27]

I suspect it wasn't used because this is an IR->MIR test; there's utils/update_mir_test_checks.py for that (works with both IR and MIR input).

[luismarques]

In D95025#2521433, @jrtc27 wrote:

I suspect it wasn't used because this is an IR->MIR test; there's utils/update_mir_test_checks.py for that (works with both IR and MIR input).

I tried to use that tool for llvm/test/CodeGen/RISCV/rvv/add-vsetvli-gpr.mir, to see if it would be practical for the growing list of RVV tests (less than 40% of them currently use it), but it crashes with:
KeyError: 'vadd_vint64m1'.
Same problem for the RVV .ll tests I tried it with, but those didn't output MIR anyway.

[jrtc27]

In D95025#2521522, @luismarques wrote:

In D95025#2521433, @jrtc27 wrote:

I suspect it wasn't used because this is an IR->MIR test; there's utils/update_mir_test_checks.py for that (works with both IR and MIR input).

I tried to use that tool for llvm/test/CodeGen/RISCV/rvv/add-vsetvli-gpr.mir, to see if it would be practical for the growing list of RVV tests (less than 40% of them currently use it), but it crashes with:
KeyError: 'vadd_vint64m1'.

That's because there's a test to print assembly. MIR input tests are intended for unit-testing passes or sets of passes; if you want to be looking at the final assembly, that should be an IR test, but the MIR after finalize-isel should be more than sufficient in almost all cases.

[frasercrmck]

Yes, we're certainly not moving away from update_llc_test_checks.py for the vector tests; I've used it in all of my work thus far. This test doesn't work perfectly with either the llc or mir scripts because it's IR -> MIR. The llc script does nothing on this test.

The mir script is more complicated, as it was used to generate the function checks in this MIR but it doesn't output anything for the YAML. So I had to add checks for the stack objects myself. I didn't want to include Assertions have been autogenerated by blah if it wasn't 100% auto-generated as it would give someone the impression they can just run the script again to regenerate the output. So I removed it. However, maybe the better approach would be to keep that line in but add an additional comment explaining this?

[jrtc27]

In D95025#2522284, @frasercrmck wrote:

Yes, we're certainly not moving away from update_llc_test_checks.py for the vector tests; I've used it in all of my work thus far. This test doesn't work perfectly with either the llc or mir scripts because it's IR -> MIR. The llc script does nothing on this test.

The mir script is more complicated, as it was used to generate the function checks in this MIR but it doesn't output anything for the YAML. So I had to add checks for the stack objects myself. I didn't want to include Assertions have been autogenerated by blah if it wasn't 100% auto-generated as it would give someone the impression they can just run the script again to regenerate the output. So I removed it. However, maybe the better approach would be to keep that line in but add an additional comment explaining this?

Hm, I guess ideally we'd have an option for update_mir_test_checks.py to emit the stack info. I'll see if I can do that later today.

[luismarques]

In D95025#2522375, @jrtc27 wrote:

Hm, I guess ideally we'd have an option for update_mir_test_checks.py to emit the stack info. I'll see if I can do that later today.

@jrtc27 Were you able to progress on that front?

[frasercrmck]

@luismarques any objections if I merge this before D99087? It contains "the original" test that's fixed by it.

[luismarques]

Once update_mir_test_checks.py is tweaked we can start using it for this test, but let's merge it as-is for now. Thanks!

llvm/test/CodeGen/RISCV/vector-abi.ll

+; RUN: llc -mtriple=riscv32 -stop-after finalize-isel < %s | FileCheck %s -check-prefix=RV32
+; RUN: llc -mtriple=riscv64 -stop-after finalize-isel < %s | FileCheck %s -check-prefix=RV64
+
+; FIXME: The stack location used to pass the parameter to the function has the
+; incorrect size and alignment for how we use it, and we clobber the stack.
+
+declare void @callee(<4 x i8> %v)
+
+define void @caller() {
+  ; RV32-LABEL: name: caller
+  ; RV32: stack:
+  ; RV32:     - { id: 0, name: '', type: default, offset: 0, size: 4, alignment: 4,
+  ; RV32-NEXT:    stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+  ; RV32-NEXT:    debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  ; RV32: bb.0 (%ir-block.0):
+  ; RV32:   ADJCALLSTACKDOWN 0, 0, implicit-def dead $x2, implicit $x2
+  ; RV32:   [[ADDI:%[0-9]+]]:gpr = ADDI $x0, 7
+  ; RV32:   SW killed [[ADDI]], %stack.0, 12 :: (store 4 into %stack.0)
+  ; RV32:   [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, 6
+  ; RV32:   SW killed [[ADDI1]], %stack.0, 8 :: (store 4 into %stack.0)
+  ; RV32:   [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 5
+  ; RV32:   SW killed [[ADDI2]], %stack.0, 4 :: (store 4 into %stack.0)
+  ; RV32:   [[ADDI3:%[0-9]+]]:gpr = ADDI $x0, 4
+  ; RV32:   SW killed [[ADDI3]], %stack.0, 0 :: (store 4 into %stack.0)
+  ; RV32:   [[ADDI4:%[0-9]+]]:gpr = ADDI %stack.0, 0
+  ; RV32:   $x10 = COPY [[ADDI4]]
+  ; RV32:   PseudoCALL target-flags(riscv-plt) @callee, csr_ilp32_lp64, implicit-def dead $x1, implicit $x10, implicit-def $x2
+  ; RV32:   ADJCALLSTACKUP 0, 0, implicit-def dead $x2, implicit $x2
+  ; RV32:   PseudoRET
+  ; RV64-LABEL: name: caller
+  ; RV64: stack:
+  ; RV64:     - { id: 0, name: '', type: default, offset: 0, size: 4, alignment: 4,
+  ; RV64-NEXT:    stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+  ; RV64-NEXT:    debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  ; RV64: bb.0 (%ir-block.0):
+  ; RV64:   ADJCALLSTACKDOWN 0, 0, implicit-def dead $x2, implicit $x2
+  ; RV64:   [[ADDI:%[0-9]+]]:gpr = ADDI $x0, 7
+  ; RV64:   SD killed [[ADDI]], %stack.0, 24 :: (store 8 into %stack.0)
+  ; RV64:   [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, 6
+  ; RV64:   SD killed [[ADDI1]], %stack.0, 16 :: (store 8 into %stack.0)
+  ; RV64:   [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 5
+  ; RV64:   SD killed [[ADDI2]], %stack.0, 8 :: (store 8 into %stack.0)
+  ; RV64:   [[ADDI3:%[0-9]+]]:gpr = ADDI $x0, 4
+  ; RV64:   SD killed [[ADDI3]], %stack.0, 0 :: (store 8 into %stack.0)
+  ; RV64:   [[ADDI4:%[0-9]+]]:gpr = ADDI %stack.0, 0
+  ; RV64:   $x10 = COPY [[ADDI4]]
+  ; RV64:   PseudoCALL target-flags(riscv-plt) @callee, csr_ilp32_lp64, implicit-def dead $x1, implicit $x10, implicit-def $x2
+  ; RV64:   ADJCALLSTACKUP 0, 0, implicit-def dead $x2, implicit $x2
+  ; RV64:   PseudoRET
+  call void @callee(<4 x i8> <i8 4, i8 5, i8 6, i8 7>)
+  ret void
+}