This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a test w/ RVV stack objects misaligning non-RVV ones
ClosedPublic

Authored by frasercrmck on May 11 2022, 6:28 AM.

Details

Summary

This patch adds a simple test which demonstrates a miscompilation of
16-byte-aligned scalar (non-RVV) objects when combined with RVV stack
objects.

The RISCV stack is assumed to be aligned to 16 bytes, and this is
guaranteed/assumed to be true when setting up the stack. However, when
the stack contains RVV objects, we decrement the stack pointer by some
multiple of vlenb, which is only guaranteed to be aligned to 8 bytes.
This means that non-RVV objects specifically requiring 16-byte alignment
fall through the cracks and are misaligned. Objects requiring larger
alignment trigger stack realignment and thus should be okay.

Diff Detail

Event Timeline

frasercrmck created this revision.May 11 2022, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 6:28 AM
frasercrmck requested review of this revision.May 11 2022, 6:28 AM

I think this can be exhibited even more simply. If we have one RVV object and call a function, that called function expects its stack to be 16-byte aligned. We're not ensuring that.

Of course, most of the time, VLEN is at least 128 so this isn't a problem. I wonder if we can use the architectural knowledge of VLEN to omit any alignment if VLEN >= 128. That doesn't fix our problem with RVV alignments > 16, but at least it brings us up to par with the scalar frame layout.

kito-cheng added inline comments.May 15 2022, 7:50 PM
llvm/test/CodeGen/RISCV/rvv/scalar-stack-align.ll
5

v implied zvl128b which guarantee VLEN is at least 128, so maybe zve32* or zve64*? they only implied zvl64b/zvl32b.

frasercrmck added inline comments.May 16 2022, 9:39 AM
llvm/test/CodeGen/RISCV/rvv/scalar-stack-align.ll
5

Yeah, you're right. In future patches I'll probably want both zve64 and v to show that with v we can rely on vlen's alignment rather than padding the stack. The question is whether I should add both RUN lines now or later?

add zve64x run lines: update comments

This revision is now accepted and ready to land.May 16 2022, 11:57 PM
This revision was landed with ongoing or failed builds.May 17 2022, 1:01 AM
This revision was automatically updated to reflect the committed changes.