Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification needed for Tag_RISCV_unaligned_access #283

Open
preames opened this issue May 26, 2022 · 7 comments
Open

Clarification needed for Tag_RISCV_unaligned_access #283

preames opened this issue May 26, 2022 · 7 comments

Comments

@preames
Copy link

preames commented May 26, 2022

This issue tracks a couple of ambiguities in the wording for this which came up in discussion on https://reviews.llvm.org/D126085.

I'm going to start with a couple of detailed concerns, and then raise a broader justification question at the end.

First, the word "impose" in "Indicates whether to impose unaligned memory accesses in code generation." is unclear to me. Its not clear what meaning this was intended to mean.

Second, giving the existence of inline assembly, it is impossible for the compiler to precisely determine if the object file contains an unaligned load. The author of the inline assembly knows, but the alignment may not be visible to the compiler in any way.

This leaves the compiler two choices: either always set the flag when a possibly unaligned load is present, or don't.

Both are problematic.

If we set it conservatively (which seems to match the "may perform" wording), then essentially every object file will have this set. This means that runtime could can not take any meaning from the tag, as while the code "may" contain unaligned access, there's no guarantee that a) such unaligned access exists, b) if it exists it executes, or thus c) that the hardware actually supports unaligned accesses.

If we only set it when we know an unaligned access exists, then tools have to deal with object files which do contain unaligned accesses, but the compiler failed to notice.

Third, the tag appears to indicate whether an unaligned access exists, not whether it executes. This is problematic for any binary which wishes to do runtime feature detection and dispatch between two versions of code - one with only aligned and one with unaligned access. How should such a binary be tagged?

Note that clarifying/changing any of the above may be a breaking change for existing implementations. We should make sure to survey the impact of any changes on existing deployed code before deciding on a path forward here.

Now, switching to higher level justification.

I do not understand what the intended use case for this feature is, and that makes it hard to figure out what the intended semantics are.

During the LLVM risc-v sync up call today, two possible use cases were mentioned:

  1. Using the tag to warn about likely bad binaries for a system which does not support fast unaligned access.
  2. Using the tag to change code generation policy in a debugger such as LLDB.

Use case 1 becomes nearly useless if we adopt the "may perform" interpretation above as any object file containing inline assembly will be marked as having unaligned accesses.

Use case 2 seems like a misuse of the feature. LLDB should not be generating code based on the characteristics of the binary being run, but based on the characteristics of the remote host. As an illustrative example, say you're debugging a fault due to a misaligned load executed on a remote host which does not support misaligned access. Having the debugger generate code for an expression using an unaligned load just because the ELF binary happened to use this tag seems... unhelpful.

My personal take here is that we'd likely be better removing this feature from the spec entirely. We can reserve the ID space if desired, but if this hasn't actually been implemented in any toolchain yet - which I hope is the actual status - we should defer the specification work until we have a concrete use case in mind.

@jrtc27
Copy link
Collaborator

jrtc27 commented May 26, 2022

@jrtc27
Copy link
Collaborator

jrtc27 commented May 26, 2022

@cmuellner
Copy link
Collaborator

Currently, this essentially says "the code has been compiled with -mno-strict-align".
Object files need to state if they require a machine that can handle misaligned accesses.
The idea is to prevent incompatible binaries from being started on machines that don't provide the required capabilities.
Connecting that to -mstrict-align is too conservative, but tools are free to do better as you propose
(e.g. require to emit at least one unaligned access being emitted).

It is correct, that the dynamic dispatching case is not covered yet.
But we have a limited solution using the .option directive for asm code:

.option push
.option arch,+zbb
.globl orcb
.type orcb,@function
foo:
        orc.b a0, a0
        ret
.size foo,.-foo
.option pop

This compiles without zbb in the march string and will not add zbb to Tag_RISCV_arch.
With this feature, I've been able to create a patchset that implements ifunc routines in glibc
(for str* and mem* functions).

The reason, why the dynamic dispatching aspect is not fully covered by specifications and tools atm is,
that we currently don't have run-time detection of HW capabilities in RISC-V.
However, that's one of the goals of this year (the configuration TG is working on a Unified Discovery specification).

Another aspect that I'd like to throw in here is that the upcoming profiles specification defines the Zicclsm extension,
which requires that "misaligned loads and stores to main memory regions with both the cacheability and coherence PMAs must be supported.". Dynamic dispatchers might want to check the availability of this extension.

@palmer-dabbelt
Copy link
Contributor

Phillip pointed me at this one, and while I'm not entirely sure about the LLVM side of things here's some comments:

  • The Tag_RISCV_unaligned_access is defined as "Indicates whether to impose unaligned memory accesses in code generation", but that really doesn't make any sense:
  • The Linux uABI (and SBI, for that matter) mandates that misaligned accesses are supported. This used to be required by the ISA but that was relaxed, it's not documented anywhere else.
  • Dynamic dispatchers really need to know if misaligned accesses are fast, not just supported. There's way more than one bit of information there, so we're probably going to end up with this just being a uarch-specific tuning.
  • Tag_RISCV_arch is too vague to encode the actual constraints on a binary, as it doesn't account for things like partial-file arch options and dynamic execution. We stopped ascribing any meaning to it in BFD a while ago because of that.

So IMO the right way to go here is:

  • Make linux/Documentation/riscv/uabi.rst and write down that misaligned access rule.
  • Have LLDB handle misaligned accesses for Linux targets, quoting said rule.
  • Add something to the GDB XML file (I'm assuming LLDB can read those too?) that says whether misaligned accesses are supported, which the remote can then fill out for embedded targets (maybe just by knowing that, or maybe by probing).
  • Deprecate Tag_RISCV_unaligned_access, as it doesn't convey any meaningful information.

palmer-dabbelt added a commit to palmer-dabbelt/riscv-elf-psabi-doc that referenced this issue Jun 3, 2022
@kito-cheng
Copy link
Collaborator

The intention is tag that when compiler might generating unaligned access, the inline assembly code are not covered, user/programmer should take responsibility for that, that is designed for debugging performance issue or unexpected trap in some scenarios - mainly used for exclude some possibility.

What if we got unexpected slow performance or unexpected unaligned access trap? that could cause by different reason, one of the reason is compiler generated unaligned memory access - if so we can quickly diagnosis that by the checking the tag,

However profile has mention a new extension name called Zicclsm recently, that extension indicated misaligned loads and stores is supported, so I think that would be better way to describe the info in future, just carry those info by ISA string, but the problem is we don't have too much further info for that yet and no SW has implement yet, but once we start to implement that, that should be a better way to describe the binary (via Tag_RISCV_arch).

@kito-cheng
Copy link
Collaborator

@palmer-dabbelt: I think we should give few more word to clarify Tag_RISCV_arch, the design goal of this attribute is describing the minimal execution environment requirement of the object.

@asb
Copy link
Collaborator

asb commented Aug 1, 2022

I think that Zicclsm is the right direction (and always thought it odd that support for misaligned access was left out of the ISA naming string). So although we could debate precise semantics of Tag_RISCV_unaligned_access, it would perhaps be better to update the spec with a note that this tag is expected to be deprecated once an extension is defined to indicate support for misaligned access (meaning the ISA naming string can be used instead).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants