[RISC-V] extra `andi` instruction when loading a `bool` atomically

[aoates]

When loading a bool atomically in C, clang generates an extra andi instruction, which I believe is extraneous.

Consider this small litmus test (also at https://godbolt.org/z/TzzxGra86):

#include <stdbool.h>
extern bool active;
bool foo1(void) {
  return __atomic_load_n(&active, __ATOMIC_RELAXED);
}
bool foo2(void) {
  return active;
}

With -O1, clang generates the following:

foo1:
.Lpcrel_hi0:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi0)(a0)
        lb      a0, 0(a0)
        andi    a0, a0, 1
        ret

foo2:
.Lpcrel_hi1:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi1)(a0)
        lbu     a0, 0(a0)
        ret

The atomic load version does an extra andi rather than just lbu as in the non-atomic version.

It seems GCC used to do this as well, but it was fixed (reference below).

References:

• old GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97417
  Comparison's with the same litmus test as that bug report:
• clang trunk comparison (extra andi): https://godbolt.org/z/1nzdxT64v
• GCC 14.2.0 comparison (identical output): https://godbolt.org/z/75fjrxa45
• GCC 8.2 comparison (also has extra andi): https://godbolt.org/z/9Y6osTqqP

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Andrew Oates (aoates)

When loading a `bool` atomically in C, clang generates an extra `andi` instruction, which I believe is extraneous.

Consider this small litmus test (also at https://godbolt.org/z/TzzxGra86):

#include <stdbool.h>
extern bool active;
bool foo1(void) {
  return __atomic_load_n(&active, __ATOMIC_RELAXED);
}
bool foo2(void) {
  return active;
}

With -O1, clang generates the following:

foo1:
.Lpcrel_hi0:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi0)(a0)
        lb      a0, 0(a0)
        andi    a0, a0, 1
        ret

foo2:
.Lpcrel_hi1:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi1)(a0)
        lbu     a0, 0(a0)
        ret

The atomic load version does an extra andi rather than just lbu as in the non-atomic version.

It seems GCC used to do this as well, but it was fixed (reference below).

References:

• old GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97417
  Comparison's with the same litmus test as that bug report:
• clang trunk comparison (extra andi): https://godbolt.org/z/1nzdxT64v
• GCC 14.2.0 comparison (identical output): https://godbolt.org/z/75fjrxa45
• GCC 8.2 comparison (also has extra andi): https://godbolt.org/z/9Y6osTqqP

[topperc]

I glanced at this briefly. I don't think the frontend is providing any range metadata that could be used to make this optimization.

[aoates]

It seems to be specific to bool as well; other integer types of different sizes don't have this issue. It's not specific to RISC-V though, it looks like at least x86-64 has the same problem.

[jrtc27]

For a normal bool load clang will generate load i8 followed by trunc nuw i8 to i1, but for this atomic load it fails to include the nuw for the trunc. Adding it manually to the IR generates the desired assembly for x86, though it seems RISC-V ends up failing to fold the zero-extend into the lb as an lbu.

EDIT: Oh nevermind I see what you mean now, the nuw gets added later by IPSCCP thanks to the !range metadata on the non-atomic load. Adding it manually to the IR does indeed let the nuw be inferred by opt.

[dtcxzyw]

EDIT: Oh nevermind I see what you mean now, the nuw gets added later by IPSCCP thanks to the !range metadata on the non-atomic load. Adding it manually to the IR does indeed let the nuw be inferred by opt.

We can add !range metadata to the atomic load in AtomicInfo::EmitAtomicLoadOp. See also

llvm-project/clang/lib/CodeGen/CGExpr.cpp

Lines 2045 to 2050 in f4043f4

	} else if (CGM.getCodeGenOpts().OptimizationLevel > 0)
	if (llvm::MDNode *RangeInfo = getRangeForLoadFromType(Ty)) {
	Load->setMetadata(llvm::LLVMContext::MD_range, RangeInfo);
	Load->setMetadata(llvm::LLVMContext::MD_noundef,
	llvm::MDNode::get(getLLVMContext(), {}));
	}

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Andrew Oates (aoates)

When loading a `bool` atomically in C, clang generates an extra `andi` instruction, which I believe is extraneous.

Consider this small litmus test (also at https://godbolt.org/z/TzzxGra86):

#include <stdbool.h>
extern bool active;
bool foo1(void) {
  return __atomic_load_n(&active, __ATOMIC_RELAXED);
}
bool foo2(void) {
  return active;
}

With -O1, clang generates the following:

foo1:
.Lpcrel_hi0:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi0)(a0)
        lb      a0, 0(a0)
        andi    a0, a0, 1
        ret

foo2:
.Lpcrel_hi1:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi1)(a0)
        lbu     a0, 0(a0)
        ret

The atomic load version does an extra andi rather than just lbu as in the non-atomic version.

It seems GCC used to do this as well, but it was fixed (reference below).

References:

• old GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97417
  Comparison's with the same litmus test as that bug report:
• clang trunk comparison (extra andi): https://godbolt.org/z/1nzdxT64v
• GCC 14.2.0 comparison (identical output): https://godbolt.org/z/75fjrxa45
• GCC 8.2 comparison (also has extra andi): https://godbolt.org/z/9Y6osTqqP

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Andrew Oates (aoates)

When loading a `bool` atomically in C, clang generates an extra `andi` instruction, which I believe is extraneous.

Consider this small litmus test (also at https://godbolt.org/z/TzzxGra86):

#include <stdbool.h>
extern bool active;
bool foo1(void) {
  return __atomic_load_n(&active, __ATOMIC_RELAXED);
}
bool foo2(void) {
  return active;
}

With -O1, clang generates the following:

foo1:
.Lpcrel_hi0:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi0)(a0)
        lb      a0, 0(a0)
        andi    a0, a0, 1
        ret

foo2:
.Lpcrel_hi1:
        auipc   a0, %got_pcrel_hi(active)
        ld      a0, %pcrel_lo(.Lpcrel_hi1)(a0)
        lbu     a0, 0(a0)
        ret

The atomic load version does an extra andi rather than just lbu as in the non-atomic version.

It seems GCC used to do this as well, but it was fixed (reference below).

References:

• old GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97417
  Comparison's with the same litmus test as that bug report:
• clang trunk comparison (extra andi): https://godbolt.org/z/1nzdxT64v
• GCC 14.2.0 comparison (identical output): https://godbolt.org/z/75fjrxa45
• GCC 8.2 comparison (also has extra andi): https://godbolt.org/z/9Y6osTqqP