Skip to content

LLVM assertion when compiling core for the MSP430 architecture #37829

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 Nov 17, 2016 · 9 comments
Closed

LLVM assertion when compiling core for the MSP430 architecture #37829

japaric opened this issue Nov 17, 2016 · 9 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.

Comments

@japaric
Copy link
Member

japaric commented Nov 17, 2016

STR

$ cat msp430.json
{
  "arch": "msp430",
  "data-layout": "e-m:e-p:16:16-i32:16:32-a:16-n8:16",
  "executables": true,
  "linker": "msp430-elf-gcc",
  "llvm-target": "msp430",
  "max-atomic-width": 0,
  "no-integrated-as": true,
  "os": "none",
  "panic-strategy": "abort",
  "relocation-model": "static",
  "target-endian": "little",
  "target-pointer-width": "16"
}

$ rustup component add rust-src

$ rustc --target msp430 $(rustc --print sysroot)/lib/rustlib/src/rust/src/libcore/lib.rs
rustc: /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/llvm/lib/Support/APInt.cpp:939: llvm::APInt llvm::APInt::trunc(unsigned int) const: Assertion `width < BitWidth && "Invalid APInt Truncate request"' failed.

Meta

$ rustc -V
rustc 1.15.0-nightly (43006fcea 2016-11-15)

I didn't see this when I tested #37672 because the default rustbuild configuration has LLVM assertions disabled whereas nightlies enable the assertions. 😞

cc @alexcrichton

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

We've wondered from time to time whether to turn LLVM assertions on during development, but most of the time we conclude "no" as it slows down builds and it's quite rare for them to trigger during typical development.

Should write down somewhere that when adding a new target it's highly recommended to do so!

@pftbest
Copy link
Contributor

pftbest commented Nov 18, 2016

Minimal IR that triggers this assertion:
bugpoint-reduced-simplified.ll

$ llc bugpoint-reduced-simplified.ll 
Assertion failed: (width < BitWidth && "Invalid APInt Truncate request"), function trunc, file /Users/vadzim/llvm/lib/Support/APInt.cpp, line 917.
Stack dump:
0.  Program arguments: llc bugpoint-reduced-simplified.ll 
1.  Running pass 'Function Pass Manager' on module 'bugpoint-reduced-simplified.ll'.
2.  Running pass 'MSP430 DAG->DAG Pattern Instruction Selection' on function '@_ZN4core3num6bignum5tests6Big8x37div_rem17h4a91efdf5fc031b4E'
Abort trap: 6

@shepmaster
Copy link
Member

shepmaster commented Nov 18, 2016

Looks very similar to an issue that AVR ran into. I've had this patch that I've been applying manually, not sure how valid it is to upstream.

@pftbest
Copy link
Contributor

pftbest commented Nov 18, 2016

@shepmaster
I don't think this is a good solution to just hide the symptoms, the real issue will remain, and probably cause more harm in the future.

I did some digging and found out that in this case umul gets legalized incorrectly, and in result zero extend from i16 to i16 is generated. And such zero extends should not exist in DAG, thats why this assertion fires.

@shepmaster
Copy link
Member

I don't think this is a good solution to just hide the symptoms

Absolutely, which is why I haven't submitted it anywhere; I was simply trying to help. I apologize.

I would like to see what changes you make to LLVM to fix this, as it's likely AVR will need the same thing. You might even fix it, now that AVR is in LLVM :-).

@pftbest
Copy link
Contributor

pftbest commented Nov 18, 2016

I was simply trying to help. I apologize.

I didn't mean to be rude. I'm sorry if it sounded that way.

I would like to see what changes you make to LLVM to fix this, as it's likely AVR will need the same thing.

Of course, I will try fix this, but I'm still new to LLVM so can't promise anything. Maybe aKor from #llvm (author of MSP430 backend) will help, he already knows about this issue.

@pftbest
Copy link
Contributor

pftbest commented Nov 28, 2016

I have posted the fix for review:
https://reviews.llvm.org/D27154
It fixes the assertion, but the resulting assembly code looks very bad, possibly incorrect, so I think more work is needed still.

llvm-beanz pushed a commit to llvm-beanz/llvm-submodules that referenced this issue Dec 6, 2016
On some platforms (like MSP430) the second element of the result
structure for SMULO/UMULO may have a shorter type than the one
returned by SetCC. We need to truncate it to the right type, or
else some incorrect code may be generated later on.

This fixes issue rust-lang/rust#37829

Patch by Vadzim Dambrouski!

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@288857 91177308-0d34-0410-b5e6-96231b3b80d8
earl pushed a commit to earl/llvm-mirror that referenced this issue Dec 6, 2016
On some platforms (like MSP430) the second element of the result
structure for SMULO/UMULO may have a shorter type than the one
returned by SetCC. We need to truncate it to the right type, or
else some incorrect code may be generated later on.

This fixes issue rust-lang/rust#37829

Patch by Vadzim Dambrouski!

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



git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@288857 91177308-0d34-0410-b5e6-96231b3b80d8
@pftbest
Copy link
Contributor

pftbest commented Dec 7, 2016

Fixed in llvm upstream, commit rL288857.
Don't know if I should backport it to rust-llvm, or should we wait for llvm 4?

@japaric
Copy link
Member Author

japaric commented Dec 7, 2016

@pftbest I'd say backport if it's easy to do so. No idea how long the next llvm-up will take.

pftbest pushed a commit to pftbest/rust that referenced this issue Dec 8, 2016
jyknight pushed a commit to jyknight/llvm-monorepo-old1 that referenced this issue Dec 10, 2016
On some platforms (like MSP430) the second element of the result
structure for SMULO/UMULO may have a shorter type than the one
returned by SetCC. We need to truncate it to the right type, or
else some incorrect code may be generated later on.

This fixes issue rust-lang/rust#37829

Patch by Vadzim Dambrouski!

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

llvm-svn=288857
bors added a commit that referenced this issue Dec 11, 2016
LLVM: Update submodule to include patches for MSP430.

Fixes #37829
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
Projects
None yet
Development

No branches or pull requests

4 participants