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

Add -m[no-]unaligned-access #62

Closed
wants to merge 4 commits into from

Conversation

wangpc-pp
Copy link

@wangpc-pp wangpc-pp commented Dec 26, 2023

These two options are negative equivalent to -m[no-]strict-align.
Clang/LLVM has supported both ways (same as AArch32/AArch64/LoongArch).
This can reduce some miscompiles when migrating some existed software packages.

These two options are negative equivalent to -m[no-]strict-align.

Signed-off-by: Wang Pengcheng <wangpengcheng.pp@bytedance.com>
riscv-c-api.md Outdated Show resolved Hide resolved
Signed-off-by: Wang Pengcheng <wangpengcheng.pp@bytedance.com>
Copy link
Contributor

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this patch is missing an explanation of why we'd want two ways of doing the same thing. It's probably clear to you, or else you wouldn't have proposed it, but it isn't clear to me.

@wangpc-pp
Copy link
Author

wangpc-pp commented Dec 26, 2023

I think this patch is missing an explanation of why we'd want two ways of doing the same thing. It's probably clear to you, or else you wouldn't have proposed it, but it isn't clear to me.

Oh sorry! The background is: Clang/LLVM has supported both ways (same as AArch32/AArch64/LoongArch).

def munaligned_access : Flag<["-"], "munaligned-access">, Group<m_Group>,
  HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64/LoongArch/RISC-V only)">;
def mno_unaligned_access : Flag<["-"], "mno-unaligned-access">, Group<m_Group>,
  HelpText<"Force all memory accesses to be aligned (AArch32/AArch64/LoongArch/RISC-V only)">;

def mstrict_align : Flag<["-"], "mstrict-align">, Alias<mno_unaligned_access>,
  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
  HelpText<"Force all memory accesses to be aligned (same as mno-unaligned-access)">;
def mno_strict_align : Flag<["-"], "mno-strict-align">, Alias<munaligned_access>,
  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
  HelpText<"Allow memory accesses to be unaligned (same as munaligned-access)">;

This can reduce some miscompiles when migrating some existed software packages.
And I just posted the GCC implementation recently.

@aswaterman
Copy link
Contributor

@wangpc-pp I am not qualified to comment on whether that's a sufficient justification, but my point was, the explanation needs to go into the document. Perhaps it could be a non-normative note, but regardless, readers will want to understand why there are two ways to do the same thing.

Signed-off-by: Wang Pengcheng <wangpengcheng.pp@bytedance.com>
Signed-off-by: Wang Pengcheng <wangpengcheng.pp@bytedance.com>
@wangpc-pp
Copy link
Author

Ping for comments. :-)

@MaskRay
Copy link
Contributor

MaskRay commented Jan 10, 2024

I think this patch is missing an explanation of why we'd want two ways of doing the same thing. It's probably clear to you, or else you wouldn't have proposed it, but it isn't clear to me.

Oh sorry! The background is: Clang/LLVM has supported both ways (same as AArch32/AArch64/LoongArch).

I don't think this is sufficient motivation.

It's unfortunate that Clang uses Alias and not make the options target-specific.
Well, if an architecture wants just one spelling, we can make the option target-specific and drop Alias.

@wangpc-pp wangpc-pp closed this Jan 25, 2024
MaskRay added a commit to llvm/llvm-project that referenced this pull request Mar 15, 2024
GCC ports only supports one of the options, with -mstrict-align
preferred by newer ports. They reject adding -m[no-]unaligned-access to
newer ports that use -m[no-]strict-align.
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111555).

We should not support aliases, either. Since the behavior has been
long-time for ARM (a146a48), support
both forms for ARM for now but remove -m[no-]unaligned-access for
RISC-V/LoongArch (see also
riscv-non-isa/riscv-c-api-doc#62).

While here, add TargetSpecific to ensure errors on unsupported targets
(https://reviews.llvm.org/D151590) and remove unneeded CC1 options.

Pull Request: #85350
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

Successfully merging this pull request may close these issues.

None yet

3 participants