Skip to content

Experimental AVR target improvements #1

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

Merged

Conversation

gergoerdi
Copy link
Collaborator

Noob alert: Please review very carefully -- I haven't even looked at LLVM's internals until a week ago.

The commits should be fairly self-explanatory; the fixes are for the following issues:

@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch 2 times, most recently from 05e9b65 to 5e80213 Compare May 7, 2017 06:32
@shepmaster
Copy link
Member

Thanks for the patches! I think our preference is to land everything in upstream LLVM before it finds its way into this repository, but this seems like a good place to track those patches before then (and also to allow @dylanmckay to grab them and review them)

Said another way: I think this PR can stay open, but not be merged until each of the commits has been merged into LLVM.

@gergoerdi
Copy link
Collaborator Author

Oh, I thought it was exactly the other way round -- that this repo is to stage dubious changes until they can be upstreamed to LLVM.

BTW, most definitely don't merge this PR, since the last three commits break structure unpacking: avr-rust/rust-legacy-fork#48

@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch 2 times, most recently from bf415da to 9fe62d7 Compare May 10, 2017 07:44
@gergoerdi
Copy link
Collaborator Author

This branch now has a ton of useful fixes with which I was able to compile a non-trivial Rust app to AVR (I hope to write a blog post about it shortly). Can I leave it in your hands to upstream it to LLVM proper?

@shepmaster
Copy link
Member

@gergoerdi each separate fix to LLVM should have one or more tests associated with it; otherwise it's highly likely that any fixes will regress with time.

@dylanmckay any other things you'd like to see?

@gergoerdi
Copy link
Collaborator Author

@shepmaster OK I'll try to find the time to add tests for the new fixes. My question still stands though -- can you or @dylanmckay take care of the upstreaming? I'm not yet invested enough in this to want to take on that hassle.

@gergoerdi
Copy link
Collaborator Author

gergoerdi commented May 12, 2017

Also, I think there's three categories of my commits here:

  1. Definite bugfixes, no reason not to merge them:
  1. Definite improvement, but implementation could be better
  1. Desperate struggling piling hacks upon hacks fighting the Harvard architecture

I don't think commits from the third category should be merged; I'll open a separate ticket where we can hopefully come up with a proper solution to PROGMEM-related issues.

@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch from 65f3f2a to 4e1e3f0 Compare May 12, 2017 12:40
@dylanmckay
Copy link
Member

I'm happy to take on upstreaming @gergoerdi, will keep you updated.

@@ -2057,6 +2109,10 @@ def : Pat<(store i8:$src, (i16 (AVRWrapper tglobaladdr:$dst))),
def : Pat<(store i16:$src, (i16 (AVRWrapper tglobaladdr:$dst))),
(STSWKRr tglobaladdr:$dst, i16:$src)>;

// Temporary workaround -- this one is BOGUS!
Copy link
Member

Choose a reason for hiding this comment

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

Why is this temporary? What is it working around? Can you pop a comment somewhere with an explanation?

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
… same spot

Contributed by Dr. Gergő Érdi.

Fixes a bug.

Raised from (avr-rust/rust-legacy-fork#49).

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@302973 91177308-0d34-0410-b5e6-96231b3b80d8
@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch 2 times, most recently from 9b6cd32 to f4d1897 Compare May 20, 2017 16:12
@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch 2 times, most recently from 5e0418c to e65b858 Compare May 28, 2017 10:40
dylanmckay and others added 12 commits May 31, 2017 18:26

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
(avr-rust/rust-legacy-fork#50)

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@304283 91177308-0d34-0410-b5e6-96231b3b80d8

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
When generating code for a shift loop, check the shift
 amount against the literal value 0, not R0

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@304284 91177308-0d34-0410-b5e6-96231b3b80d8

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
Summary: CPI does not read the status register, but only writes it.

Reviewers: dylanmckay

Reviewed By: dylanmckay

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D33223

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@304116 91177308-0d34-0410-b5e6-96231b3b80d8

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
…e same

spot as the original MBB (avr-rust/rust-legacy-fork#62)

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove

Verified

This commit was signed with the committer’s verified signature.
dgrove-oss David Grove
For now, these relaxed branches are used unconditionally, even when
the branch target is nearby.
avr-rust/rust-legacy-fork#44
FIXME: implementation is mostly copy-pasted from LDWRdPtr, so we should
refactor a bit and unify the two
@gergoerdi gergoerdi force-pushed the avr-rust-experiments branch from e65b858 to f3dcdf8 Compare June 17, 2017 02:03
@dylanmckay
Copy link
Member

It looks like there's some stuff here that still needs to be merged

I will change the base branch to see more clearly what is missing that should go into master

@dylanmckay dylanmckay changed the base branch from avr-rust-2017-05-03 to avr-rust-llvm-release-4-0-1 October 4, 2017 09:27
@dylanmckay
Copy link
Member

That didn't help, the histories are different!

@dylanmckay dylanmckay changed the base branch from avr-rust-llvm-release-4-0-1 to avr-rust-2017-05-03 October 4, 2017 09:28
@dylanmckay
Copy link
Member

Stuff to merge

  • "[AVR] Elaborate LDWRdPtr into ld r, X++; ld r+1, X"
  • Grab mayLoad change from the diff out of "[AVR] Add ISel pattern which maches load-from-global, compile it to LPMRdZ"
  • "[AVR] Implement LPMWRdZ pseudo-instruction's expansion."

I believe everything else is already upstreamed, or were temporary hacks

@dylanmckay
Copy link
Member

Ran tests locally, all pass

@dylanmckay dylanmckay merged commit 634f5a7 into avr-rust:avr-rust-2017-05-03 Oct 4, 2017
@dylanmckay
Copy link
Member

Wrong merge request! did not mean to merge, nor did I run tests for this PR.

Reopening now, have reverted

@dylanmckay
Copy link
Member

Looks like there's no reopen button, will just cherry-pick stuff now

@dylanmckay
Copy link
Member

dylanmckay commented Oct 4, 2017

Upstreamed ""[AVR] Elaborate LDWRdPtr into ld r, X++; ld r+1, X"" in r314896.

I also fixed a bunch of tests before committing

@dylanmckay
Copy link
Member

Upsteamed mayLoad factoring out in r314897

@dylanmckay
Copy link
Member

Upsteamed "[AVR] Implement LPMWRdZ pseudo-instruction's expansion." in r314898

@dylanmckay
Copy link
Member

Cherry-picked the upstreamed commits to avr-rust/llvm in

06d0f6b [AVR] Implement LPMWRdZ pseudo-instruction's expansion.
f6d00a7 [AVR] Factor out mayLoad in tablegen patterns
d4274bd [AVR] Elaborate LDWRdPtr into ld r, X++; ld r+1, X

dylanmckay added a commit to avr-rust/rust-legacy-fork that referenced this pull request Oct 4, 2017
@dylanmckay
Copy link
Member

dylanmckay commented Oct 4, 2017

Updated version of LLVM in for Rust in avr-rust/rust-legacy-fork@a22dd46

@dylanmckay
Copy link
Member

Added these commits to the current AVR 5.0.1 merge request PR34829

@dylanmckay
Copy link
Member

Have double checked all the commits in this PR and everything looks good.

Thanks for the patches @gergoerdi, sorry for taking so long, completely forgot about this PR!

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