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

Why is there alignment in the LLVM datalayout? #64

Closed
gergoerdi opened this issue May 24, 2017 · 7 comments
Closed

Why is there alignment in the LLVM datalayout? #64

gergoerdi opened this issue May 24, 2017 · 7 comments

Comments

@gergoerdi
Copy link
Collaborator

@gergoerdi gergoerdi commented May 24, 2017

I've tracked down a bug to bad alignment here: #63 (comment) After a lot of pain, I've found out that there is actually a hardcoded datalayout in the AVR backend, in AVRTargetMachine.cpp, taking precedence over whatever the user puts in the LLVM IR file. This datalayout is currently as follows:

static const char *AVRDataLayout = "e-p:16:16:16-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-n8";

Expanded, the above states:

  • 2-byte words are 2-byte-aligned, 4-byte words are 4-byte-aligned, 8-byte words are 8-byte-aligned
  • floats are 4-byte-aligned, doubles are 8-byte-aligned
  • Pointers take up 2 bytes and are 2-byte-aligned
  • Aggregates are 8-byte-aligned

To my untrained eye, this looks completely bogus (except, yeah, pointers are 2 bytes:)), so I did a quick git blame in the hope that I'd find a good reason. There is a commit which gives an explanation for the 2-byte alignment:

commit 553362942dbc6baa23225d7e39020631a9c15b91
Author: Dylan McKay <...>
Date:   Wed Sep 28 13:29:10 2016 +0000

    [AVR] Update the data layout
    
    The previous data layout caused issues when dealing with atomics.
    
    Foe example, it is illegal to load a 16-bit value with less than 16-bits
    of alignment.
    
    This changes the data layout so that all types are aligned by at least
    their own width.
    
    Interestingly, this also _slightly_ decreased register pressure in some
    cases.
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@282587 91177308-0d34-0410-b5e6-96231b3b80d8

but it still doesn't explain what's going on with the 4- and even 8-byte alignments (which were also introduced in the same commit). The original code is already missing an a specifier, so that's probably just an oversight/typo.

If we need 2-byte alignment for >=2 byte words as the above commit states, what's the reason for going above that for wider types, that are going to be soft-implemented anyway?

Would this work instead? It would certainly fix #63

static const char *AVRDataLayout = "e-p:16:16-i8:8-i16:16-i32:16-i64:16-f32:16-f64:16-n8-a:16";
@gergoerdi
Copy link
Collaborator Author

@gergoerdi gergoerdi commented May 25, 2017

@gergoerdi
Copy link
Collaborator Author

@gergoerdi gergoerdi commented May 27, 2017

On StackOverflow, user tofro writes in his answer:

This comment doesn't make any sense to me. AVR does not natively know 16-bit-types, and specifically not atomic access to 16-bit-types, regardless of any alignment.

On a classic AVR CPU, alignment of 16 bit data is not required as 16-bit memory access always evaluates to two 8-bit fetches to two registers.

There is a kind of "alignment restriction", though, when using the movw instruction available on some AVRs that moves data from one register pair to another - Here the register number of the lower registers must be even. This has nothing to do with memory alignment, though.

I'd like to try compiling LLVM/rustc with 8-bit alignment through and through and see how it goes. @dylanmckay, do you have a ticket # or a repro description or anything on the problems you've encountered that triggered the above commit?

@gergoerdi
Copy link
Collaborator Author

@gergoerdi gergoerdi commented May 27, 2017

As a test, I've now recompiled libcore-mini + chip8-engine + chip8-avr with 8-bit alignment, and everything seems to work completely fine.

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 27, 2017
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue May 28, 2017
@dylanmckay
Copy link
Member

@dylanmckay dylanmckay commented May 31, 2017

Most of the alignment stuff has been untouched since I originally imported the old SVN repository from SourceForge. I haven't dealt with it much and so my knowledge is pretty poor.

Safest to assume that if something looks intentional, it probably isn't 😜

@dylanmckay, do you have a ticket # or a repro description or anything on the problems you've encountered that triggered the above commit?

From memory, I think any atomic-related code would trigger it. I think the related issue is avr-llvm/llvm#214.

gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue Jun 17, 2017
@gergoerdi
Copy link
Collaborator Author

@gergoerdi gergoerdi commented Jun 18, 2017

I have now pushed the changes on both the LLVM and the Rust side that turn off all alignment.

@gergoerdi gergoerdi closed this Jun 18, 2017
gergoerdi added a commit to gergoerdi/llvm-avr that referenced this issue Jun 18, 2017
dylanmckay added a commit to avr-rust/llvm that referenced this issue Sep 24, 2017
chapuni pushed a commit to llvm-project/llvm-project-20170507 that referenced this issue Sep 26, 2017
This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
chapuni pushed a commit to llvm-project/llvm that referenced this issue Sep 26, 2017
This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314179 91177308-0d34-0410-b5e6-96231b3b80d8
@dylanmckay
Copy link
Member

@dylanmckay dylanmckay commented Sep 26, 2017

My check-everything-upstreamed.rb script found that I never ended up upstreaming the fix for this to LLVM proper, doing now

@dylanmckay
Copy link
Member

@dylanmckay dylanmckay commented Sep 26, 2017

Committed in r314179

earl pushed a commit to earl/llvm-mirror that referenced this issue Sep 26, 2017
This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314179 91177308-0d34-0410-b5e6-96231b3b80d8
chapuni pushed a commit to llvm-project/llvm-project-submodule that referenced this issue Sep 26, 2017
This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
chapuni pushed a commit to llvm-project/llvm that referenced this issue Sep 28, 2017
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------


git-svn-id: https://llvm.org/svn/llvm-project/llvm/branches/release_50@314379 91177308-0d34-0410-b5e6-96231b3b80d8
jyknight pushed a commit to jyknight/llvm-monorepo that referenced this issue Sep 28, 2017
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------

llvm-svn=314379
chapuni pushed a commit to llvm-project/llvm-project-20170507 that referenced this issue Jan 8, 2018
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------
chapuni pushed a commit to llvm-project/llvm-project-submodule that referenced this issue Jan 8, 2018
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------
llvm-git-migration pushed a commit to llvm-git-prototype/llvm that referenced this issue Nov 6, 2018
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------

llvm-svn=314379
llvm-git-migration pushed a commit to llvm-git-prototype/llvm that referenced this issue Jan 4, 2019
------------------------------------------------------------------------
r314179 | dylanmckay | 2017-09-26 13:45:27 +1300 (Tue, 26 Sep 2017) | 11 lines

[AVR] Use 1-byte alignment for all data types

This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.
------------------------------------------------------------------------

llvm-svn: 314379
JohnHolmesII pushed a commit to JohnHolmesII/llvm-project that referenced this issue Oct 12, 2020
This was an oversight in the original backend data layout.

The AVR architecture does not have the concept of unaligned loads - all
loads/stores from all addresses are aligned to one byte.

Discovered in avr-rust issue #64
avr-rust/rust-legacy-fork#64

Patch By Gergo Erdi.

llvm-svn: 314179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants