Lukacma wrote:
Hello Kerry ! The code looks good for me, but when using AI to identify issues
with this patch, multiple issues popped up. I have checked them and they seem
like valid concerns, which should be addressed before the patch is merged. Let
me know what you think or if you have any issues with reproducing this!
### P1: Stack-addressed hinted stores can crash frame-index elimination
```cpp
extern void escape(int *);
void stack_hint(int value) {
int slot;
__builtin_arm_atomic_store_with_hint(
&slot, value, __ATOMIC_RELAXED, 0);
escape(&slot);
}
```
This can select a pseudo shaped like:
```text
ATOMIC_STORE_HINT_Wui frame-index, data, offset, relaxed, hint
```
The frame-index rewrite expects the immediate immediately after the frame
index, but operand 1 is the data register. These pseudos are also missing from
`getMemOpInfo`, so elimination currently reaches `llvm_unreachable`.
Relevant code:
- `llvm/lib/Target/AArch64/AArch64InstrAtomics.td:302`
- `llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:7357`
### P1: LFI object emission separates `stshh` from its store
```cpp
void lfi_hint(int *ptr, int value) {
__builtin_arm_atomic_store_with_hint(
ptr, value, __ATOMIC_RELEASE, 0);
}
```
The textual `-S` output is correct, but compiling through the integrated
assembler:
```bash
clang++ --target=aarch64_lfi -O1 -c test.cpp -o test.o
llvm-objdump -d test.o
```
produces:
```asm
stshh keep
add x28, x27, w0, uxtw
stlr w1, [x28]
```
The AsmPrinter emits the hint and store separately. The LFI MC rewriter then
inserts its address guard between them, breaking the required adjacency.
The pseudo is also estimated as 8 bytes, while hint + guard + store occupies 12
bytes.
Relevant code:
- `llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:3281`
### P1: `_Bool` and `_BitInt` use their scalar IR representation
```cpp
void store_bool(bool *ptr, bool value) {
__builtin_arm_atomic_store_with_hint(
ptr, value, __ATOMIC_RELAXED, 0);
}
void store_bitint(unsigned _BitInt(7) *ptr,
unsigned _BitInt(7) value) {
__builtin_arm_atomic_store_with_hint(
ptr, value, __ATOMIC_RELAXED, 0);
}
```
Both types have an 8-bit memory representation, so they pass Sema’s size check.
CodeGen uses `EmitScalarExpr`, however, producing atomic `i1` and `i7` stores
instead of `i8`.
The value needs to pass through `EmitToMemory`, or these types must be rejected.
Relevant code:
- `clang/lib/CodeGen/TargetBuiltins/ARM.cpp:2080`
### P1: Array-to-pointer conversion is checked but discarded
```cpp
void array_argument() {
int storage[1];
__builtin_arm_atomic_store_with_hint(
storage, 0, __ATOMIC_RELAXED, 0);
}
```
Sema successfully creates and validates the conversion from `int[1]` to `int
*`, but never attaches the converted expression with `TheCall->setArg(0, ...)`.
CodeGen consequently receives the original array expression and asserts in
`EmitPointerWithAlignment`.
Relevant code:
- `clang/lib/Sema/SemaARM.cpp:334`
### P2: Dependent and wide immediate arguments are mishandled
Value-dependent template arguments are rejected before instantiation:
```cpp
template <unsigned Order>
void dependent_order(int *ptr) {
__builtin_arm_atomic_store_with_hint(ptr, 0, Order, 0);
}
template <unsigned Hint>
void dependent_hint(int *ptr) {
__builtin_arm_atomic_store_with_hint(
ptr, 0, __ATOMIC_RELAXED, Hint);
}
void instantiate(int *ptr) {
dependent_order<__ATOMIC_RELEASE>(ptr);
dependent_hint<1>(ptr);
}
```
These values are valid integer constant expressions after instantiation, so
checking should be deferred while they are value-dependent.
Wide invalid values are also narrowed before validation:
```cpp
void wide_arguments(int *ptr) {
// Invalid value truncates to __ATOMIC_RELEASE.
__builtin_arm_atomic_store_with_hint(
ptr, 0, (1ULL << 32) + __ATOMIC_RELEASE, 0);
// Invalid value truncates to hint 0 (KEEP).
__builtin_arm_atomic_store_with_hint(
ptr, 0, __ATOMIC_RELAXED, 1ULL << 32);
}
```
Validation should operate on the full `APSInt` before converting to `unsigned`.
Conversion results for arguments 2 and 3 should also be checked for failure
before calling `.get()`.
Relevant code:
- `clang/lib/Sema/SemaARM.cpp:371`
### P2: Equivalent typedefs and volatile pointees are rejected
```cpp
typedef int aliased_int;
void typedef_argument(aliased_int *ptr, int value) {
__builtin_arm_atomic_store_with_hint(
ptr, value, __ATOMIC_RELAXED, 0);
}
void volatile_argument(volatile int *ptr, int value) {
__builtin_arm_atomic_store_with_hint(
ptr, value, __ATOMIC_RELAXED, 0);
}
```
Raw `QualType` identity rejects both calls even though the underlying data
types match.
The comparison should use canonical unqualified types. Qualifiers should then
be handled explicitly: reject `const` destinations and preserve `volatile` on
the generated store.
Relevant code:
- `clang/lib/Sema/SemaARM.cpp:365`
https://github.com/llvm/llvm-project/pull/198316
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
