Skip to content

Invalid instructions (UMAAL) are generated for the thumbv7m-none-eabi target #37227

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

Closed
japaric opened this issue Oct 17, 2016 · 10 comments
Closed
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state

Comments

@japaric
Copy link
Member

japaric commented Oct 17, 2016

UMAAL is only available in the ARMv7E-M processors but if you compile core for the thumbv7m (ARMv7-M) target, rustc/LLVM generates them for the core::num::flt2dec::strategy::grisu::format_shortest_opt function.

@japaric japaric added A-codegen Area: Code generation O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Oct 17, 2016
@japaric
Copy link
Member Author

japaric commented Oct 17, 2016

Relevant information: See the ARM Cortex-M instructions groups table under instructions sets. UMAAL is in the group of instructions that are not available for the Cortex-M3, which is the processor the thumbv7m target covers.

Building core with a explicit -C target-cpu=cortex-m3 still generates the UMAAL instruction. This seems like a bug in LLVM.

@japaric japaric added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Oct 17, 2016
@pftbest
Copy link
Contributor

pftbest commented Oct 18, 2016

Minimal IR that triggers the problem:
bugpoint-reduced-simplified.ll
It generates umaal instruction when is compiled by llc

@pftbest
Copy link
Contributor

pftbest commented Oct 22, 2016

I have submitted the fix for review:
https://reviews.llvm.org/D25890

@pftbest
Copy link
Contributor

pftbest commented Oct 27, 2016

Fixed in llvm upstream, r285278 and r285280

@pftbest
Copy link
Contributor

pftbest commented Oct 28, 2016

How does llvm version gets updated? Do I need to backport this fix to llvm 3.9 somehow, so it could land in rust?

@alexcrichton
Copy link
Member

@pftbest nice! To update LLVM there's two routes:

  1. Backport the patches to our fork, currently around the 3.9 release.
  2. Upgrade our fork to the current LLVM head

(1) is much easier than (2) typically, and we're fine with both!

@pftbest
Copy link
Contributor

pftbest commented Oct 28, 2016

So, I've rebased rust-llvm-2016-07-09 branch to match llvm version 3.9.1 and then I backported my fix on top of it. The new branch is called rust-llvm-2016-10-29 and I don't know what to do next.

@japaric
Copy link
Member Author

japaric commented Oct 28, 2016

@pftbest I think:

  • Locally, make the rust repo's llvm submodule point to your 10-29 branch and test (make check) locally.
  • If that works, ask a core member someone with push access to the rust-lang/llvm repo to create a new 10-29 branch there
  • Then send another PR to rust-lang/rust updating its llvm submodule to the new branch in rust-lang/llvm
  • Cross your fingers and hope that it passes the buildbots.

@alexcrichton
Copy link
Member

Ok I've pushed your branch to rust-lang/llvm (thanks for the rebase to 3.9.1!). As @japaric mentioned if you want to send a PR to rust-lang/rust updating the submodule I'll r+ that

@pftbest
Copy link
Contributor

pftbest commented Oct 29, 2016

Thank you! I have submitted a #37465

bors added a commit that referenced this issue Oct 31, 2016
LLVM: Update submodule to rust-llvm-2016-10-29 branch.

Fixes #37227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state
Projects
None yet
Development

No branches or pull requests

3 participants