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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In D126085#3538325 <https://reviews.llvm.org/D126085#3538325>, @reames wrote:

> 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.

Yep, just mentioning for future reference.

>> 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.

I'm not sure the attribute is used in the wild in any meaningful way right now, but I think it does have purpose. There's no guarantee that a RISC-V EEI actually provides a trap handler for misaligned accesses, so on some targets (largely embedded), compiling with `+unaligned-scalar-mem` may result in code that is unsuitable. Given there's no way to describe support for misaligned accesses in the ISA naming string, it's probably similarly useful to the other attributes from the perspective of any tooling working with ELF files. Such tooling might be used to warn / error when a likely build misconfiguration is found within a project.

If I were authoring the patch, I'd likely put the attribute wiring in the same patch (or perhaps if I really wanted to split things, have patch 1 adding the feature but only impacting the attribute, and patch 2 adding codegen tests and changes). But tastes vary and I don't think it makes a big difference for this patch, so I'm happy with whatever sequencing you prefer here.


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

https://reviews.llvm.org/D126085