[PATCH] D126085: [RISCV] Add a subtarget feature to enable unaligned scalar loads and stores

Wed May 25 12:59:20 PDT 2022

reames added a comment.

In D126085#3538293 <https://reviews.llvm.org/D126085#3538293>, @asb wrote:

> I don't think it affects this patch (as splitting scalar vs vector features for alignment makes sense regardless of the frontend option), but I had a quick look at the frontend option for this. Looks like on the GCC side it's `-mno-strict-align`. On Arm/AArch64 it's spelled `-munaligned-access` and `__ARM_FEATURE_UNALIGNED` is set. Unless there's some target generic `#define` that's set that I'm missing, it feels like it would be useful to set a define for the RISC-V case as well (and agree this with the GNU folks).

I agree this is separate.  What a target defaults to should not be blocked on what a frontend might chose to override.  I'll note that these flags don't appear to influence x86 handling of the corresponding cases, so unless this is handled somewhere generically I missed, this isn't RISCV specific in any way.

> One thing I noticed that does affect this patch (and sorry I didn't spot this earlier!), is we should be setting `Tag_RISCV_unaligned_access` when this feature is set (see here <https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#list-of-attributes>).

Er, I'm sorry, what?  Why?

I agree this spec exists, but what purpose does the attribute bit actually serve?  There's no interaction with the dynamic loader here.  It's a compiler codegen choice.

If you really want, I'm fine adding the attribute wiring (in a separate patch), but this really looks like a spec that is a) out of sync with reality, and b) serves no obvious purpose.

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

https://reviews.llvm.org/D126085