"Cannot generate unaligned atomic load"

[gergoerdi]

One of the changes in libcore-mini is the removal of the sync module. Originally, it seemed #58 was the cause of needing to remove it, but now that that one is closed, it turns out it still can't be compiled.

The offending Rust code, stripped to its essentials, looks like this:

use core::intrinsics;
use core::cell::UnsafeCell;
use core::fmt;

pub struct AtomicI16 {
    v: UnsafeCell<i16>,
}

impl fmt::Debug for AtomicI16 {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_tuple(stringify!(AtomicI16))
            .field(&unsafe{ intrinsics::atomic_load(self.v.get()) })
            .finish()
    }
}

Further stripping down the resulting IR we get

target datalayout = "e-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
target triple = "avr-unknown-unknown"

%"atomic::AtomicI16" = type { %"core::cell::UnsafeCell<i16>", [0 x i8] }
%"core::cell::UnsafeCell<i16>" = type { i16, [0 x i8] }

; Function Attrs: uwtable
define void @foo(%"atomic::AtomicI16"*) {
start:
  %_15 = alloca i16, align 1
  %1 = getelementptr inbounds %"atomic::AtomicI16", %"atomic::AtomicI16"* %0, i16 0, i32 0, i32 0
  %2 = load atomic i16, i16* %1 seq_cst, align 1
  ret void
}

which results in the error message

LLVM ERROR: Cannot generate unaligned atomic load

[gergoerdi]

The assertion is coming from the non-AVR-specific module CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:

void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
  SDLoc dl = getCurSDLoc();
  AtomicOrdering Order = I.getOrdering();
  SynchronizationScope Scope = I.getSynchScope();

  SDValue InChain = getRoot();

  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
  EVT VT = TLI.getValueType(DAG.getDataLayout(), I.getType());

  if (I.getAlignment() < VT.getSizeInBits() / 8)
    report_fatal_error("Cannot generate unaligned atomic load");

  // ...

[gergoerdi]

Commenting out that assertion, the above IR compiles to

        .text
        .file   "a.ll"
        .globl  foo
        .p2align        1
        .type   foo,@function
foo:                                    ; @foo
; BB#0:                                 ; %start
        push    r28
        push    r29
        in      r28, 61
        in      r29, 62
        sbiw    r28, 2
        in      r0, 63
        cli
        out     62, r29
        out     63, r0
        out     61, r28
        movw    r30, r24
        in      r0, 63
        cli
        ld      r24, Z
        ldd     r25, Z+1
        out     63, r0
        adiw    r28, 2
        in      r0, 63
        cli
        out     62, r29
        out     63, r0
        out     61, r28
        pop     r29
        pop     r28
        ret
.Lfunc_end0:
        .size   foo, .Lfunc_end0-foo

but I don't have the mental capacity right now to understand that. I do see lots of cli with no matching sei, though.

[dylanmckay]

I think this may be somehow related to the data layout, as that specifies alignments for all types. I know the data layout in the current AVR-rust is slightly different to the data layout in upstream LLVM trunk.

[gergoerdi]

Upstream LLVM is wrong because it thinks there's alignment on AVR. I'm afraid that visitAtomicLoad function might simply not be ready for an architecture that has no alignment (i.e. everything is 8-bit aligned).

[dylanmckay]

The current LLVM data layout says that i8 is aligned to 8-bits, i16 is aligned to 16-bits, i32 is aligned to 32-bits, and so on^[1] .

This isn't an area I know too much about, but if a type is aligned to its own size, isn't that the same as saying there is no alignment?

• [1] "e-p:16:16:16-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-n8"

[gergoerdi]

No, because e.g. 32-bit alignment for 32-bit words would mean that you can't have a 32-bit word starting at an address not divisible by 4. No alignment means you can have a 32-bit word at any address.

[dylanmckay]

I see.

I'm looking at the reference now, a quick glance gives me the impression that if we set alignment to 8, everything will be byte aligned.

Your definition of alignment would mean that all addresses must be divisible by one in this case, and therefore it would be equivalent to no alignment?

[gergoerdi]

Yes, exactly. But I thought that's what's already in our branch of LLVM, ever since avr-rust/llvm@f499a13.

[dylanmckay]

Interesting, I'm not sure why that isn't upstreamed, it really should be.

[gergoerdi]

OK but still, the issue here is that on "our" branch, where alignment is correctly 1-byte for everything, the linked assertion fails because it assumes (for some reason) that atomic access must be aligned:

  if (I.getAlignment() < VT.getSizeInBits() / 8)
    report_fatal_error("Cannot generate unaligned atomic load");

Here, VT.getSizeInBits() / 8 is 2 for a 16-bit atomic, and getAlignment() is 1 on AVR.

[wez]

Bummed to find that there is no core::sync available right now. What needs to happen before that is potentially made available again?

[dylanmckay]

I can't recall what specific bug caused us to remove the core::sync module, but we've fixed a bunch of bugs since so maybe that module can be reinstated, but I'm not sure.

[shepmaster]

What needs to happen

The commit that removes sync says:

remove sync module to work around LDWRdPtr elaboration bug

(#58)

So try reverting that change.

[shepmaster]

As a point, I tried just reverting, and it generates the same error as #26. I don't think it's related to this issue.

I've also created avr-rust/libcore#1 to track this from the libcore side.

[dylanmckay]

Agree with Shep, I will revert and push

[dylanmckay]

@wez have reverted in 5b04cc0

[wez]

with 5b04cc064b4e083e80e674c8092e773f6c0220af I get this:

LLVM ERROR: Cannot generate unaligned atomic load
error: Could not compile `core`.

:-(

[dylanmckay]

There is discussion about this issue prompted by Gergo on llvm-dev

General summary:

You might want to define a target hook (say in Target Lowering) for say allowsUnalignedAtomicMemOps() that would return false on all targets but yours. Of course, you should definitely investigate whether there is a reason for this diagnostic (i.e. whether some other passes require natural alignment).

[dylanmckay]

Have a patch for this locally, will post to reviews.llvm.org when I've run tests.

Here's the branch

[dylanmckay]

And here's the review

https://reviews.llvm.org/D39946

I do not know the reviewers of this patch so I am unsure how long it'll take to get committed, but if it's more than a few days we can cherry-pick it to avr-rust anyway temporarily until it's in LLVM trunk.

[dylanmckay]

I will cherry-pick the un-upstreamed fix into avr-rust/llvm anyway, just so the compiler is more stable for avr-rust users.

Once D39946 lands, we should revert the old commit and cherry-pick the new one (assuming there are code review changes).

[dylanmckay]

Cherry-picked the patch into avr-rust/llvm@0149e8b and updated the LLVM submodule in 48f2d58.

[dylanmckay]

This should now be fixed - I have raised #86 so we don't forget to pull in the upstream patch when it lands.