Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#7571 closed bug (fixed)

LLVM codegen does not handle integer literals in branch conditionals.

Reported by: thoughtpolice Owned by: thoughtpolice
Priority: normal Milestone:
Component: Compiler (LLVM) Version: 7.7
Keywords: llvm, codegen Cc: dterei, mad.one@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash Difficulty: Unknown
Test Case: Blocked By: #7573
Blocking: #7588, #7589, #7590 Related Tickets: #7574, #7575

Description

When compiling GHC HEAD on my ODROID-U2 ARM machine, running Ubuntu 12.10, with LLVM 3.3svn, I get this error while building part of the RTS:

linaro@linaro-ubuntu-desktop ~/code/ghc
 % "inplace/bin/ghc-stage1" -static -prof  -H32m -O -Iincludes -Iincludes/dist -Iincludes/dist-derivedconstants/header -Iincludes/dist-ghcconstants/header -Irts -Irts/dist/build -DCOMPILING_RTS -package-name rts -dcmm-lint      -i -irts -irts/dist/build -irts/dist/build/autogen -Irts/dist/build -Irts/dist/build/autogen            -optc-O2 -O2    -c rts/StgStdThunks.cmm -o rts/dist/build/StgStdThunks.p_o  You are using a new version of LLVM that hasn't been tested yet!
We will try though...
ghc-stage1: panic! (the 'impossible' happened)
  (GHC version 7.7.20130111 for arm-unknown-linux):
        LlvmCodeGen.CodeGen.genCondBranch: Cond expr not bool! (i32 1)

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

linaro@linaro-ubuntu-desktop ~/code/ghc
 % 

The reason is that StgStdThunks?.cmm does something like:

#ifdef FOO
#define THING(x) 1
#else
#define THING(x) do_something((x))
#endif

...

  if (THING(x)) {
    ...
  }

Depending on if FOO is defined, the conditional could have an expression or a literal in the branch head.

This is directly fed into the backend from the compiler. LlvmCodeGen?.CodeGen?.genLit does not properly narrow the type of the literal from i32 to i1 when generating the code, causing a compiler panic. We would have likely never seen this outside hand-written Cmm, since the code generator would otherwise trivially eliminate such branches in the optimizers.

Coincidentally, I would imagine this failure would mean that the compiler has not been able to bootstrap with LLVM for quite some time. I haven't bisected this, but I'd speculate it was some time around the switch to the NCG.

I believe I have a fix for this that I am testing on my machine.

Attachments (2)

0001-Ensure-the-LLVM-codegen-correctly-handles-literals-i.patch (5.8 KB) - added by thoughtpolice 15 months ago.
Fix reference to 'Note' in commit.
0001-Test-for-Trac-7571.patch (1.3 KB) - added by thoughtpolice 15 months ago.
Patch for testsuite.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 15 months ago by thoughtpolice

  • Status changed from new to patch

I have verified my patch fixes the build of StdStgThunks?.cmm on my ODROID. More ARM build failures are still in my way, but nonetheless this patch should be merged for everyone since it fixes compiler bootstrapping with LLVM.

I also added a hefty note explaining the change.

commit 0de9754d06c8ca57e781bb829931ff7cdc861344
Author: Austin Seipp <mad.one@gmail.com>
Date:   Sun Jan 13 04:34:05 2013 +0000

    Ensure the LLVM codegen correctly handles literals in a branch. #7571
    
    This was causing a failure to build GHC HEAD on my ODROID-U2 ARM
    machine. In general, hand-written C-- like in StgStdThunks.cmm would
    have tripped this on any platform, meaning bootstrapping with the
    LLVM backend was probably broken before.
    
    In general, we need to be sure that when generating code for literals,
    we properly narrow the type of the literal to i1. See Note [Integer
    literals and branch conditionals] in the LlvmCodeGen.CodeGen module.
    
    This fixes Trac #7571.
    
    Signed-off-by: Austin Seipp <mad.one@gmail.com>

If someone could review/merge, I'd appreciate it a lot.

Changed 15 months ago by thoughtpolice

Fix reference to 'Note' in commit.

comment:2 Changed 15 months ago by thoughtpolice

  • Cc mad.one@… added
  • Version changed from 7.6.1 to 7.7

comment:3 Changed 15 months ago by thoughtpolice

I will have a test for this shortly I hope.

Changed 15 months ago by thoughtpolice

Patch for testsuite.

comment:4 Changed 15 months ago by thoughtpolice

  • Blocked By 7573 added

The attached patch for the testsuite correctly tickles this condition and I've verified it independently on my Mac OS X machine.

commit 62067fcf7dd6b8da9fcc76677a1c1cb59e9eff1d
Author: Austin Seipp <mad.one@gmail.com>
Date:   Sun Jan 13 03:51:36 2013 -0600

    Test for Trac #7571.
    
    Signed-off-by: Austin Seipp <mad.one@gmail.com>

The patch depends on the (very simple) testsuite patch I put in #7573. So it's currently blocked.

comment:5 Changed 15 months ago by thoughtpolice

See also #7574.

comment:6 Changed 15 months ago by thoughtpolice

  • Type of failure changed from Building GHC failed to Compile-time crash

comment:7 Changed 15 months ago by thoughtpolice

comment:8 Changed 15 months ago by altaic

Indeed, building HEAD on OS X x86_64 with llvm was broken before due to this bug. Thanks for the fix!

comment:9 Changed 15 months ago by thoughtpolice

comment:10 Changed 15 months ago by dterei

Great! Thanks Austin.

Yes the switch to the NCG and also LLVM 3.2 release both haven't been tested properly so those are the bugs you are running into, let alone 3.3. This bug seems a regression from memory.

I'll try to look at these soon and merge.

comment:11 Changed 15 months ago by thoughtpolice

Thanks David! I would like to get the ARM build shaped up before 7.8 (before, eventually, GHC > 7.8 requires > 7.4 to build, and ARM becomes drastically more difficult,) so I will probably be submitting more patches as I find problems. There are a few more beyond just ARM (you can look at the other tickets I've filed,) so any help is greatly appreciated.

comment:12 Changed 15 months ago by simonmar

  • Difficulty set to Unknown

Ok by me. I think we should clarify that CmmCondBranch can have an arbitrary expression as the conditional, and it should be a word-sized integral with non-zero meaning true.

The alternative is to declare that the argument to CmmCondBranch must be a conditional, but I think that would be worse: we wouldn't be able to unconditionally rewrite "1 == 2" to zero everywhere, we would probably need dummy "ALWAYS" and "NEVER" conditionals.

comment:13 Changed 15 months ago by thoughtpolice

  • Blocking 7588 added

comment:14 Changed 15 months ago by dterei

  • Blocking 7589 added

comment:15 Changed 15 months ago by dterei

  • Blocked By 7590 added

comment:16 follow-up: Changed 15 months ago by dterei

Austin, can you explain your setup please? All these bugs you are filing and fixing are ones I fixed in some form 2 years ago. The existing llvm backend contains code to do exactly what the bug description describes, narrow arbritry expressions to a boolean when used as a conditional.

I can also currently bootstrap GHC with LLVM fine on both OSX 10.8 and Linux. (Both 64 bit).

Are you cross compiling? Or compiling on a 64bit OSX machine but producing a 32bit GHC? I think the backend may have bugs to do with this as it makes assumptions about word sizes... and so detection of various cases that need to be avoided aren't firing.

comment:17 Changed 15 months ago by thoughtpolice

David, I gave an extensive reply in #7580, please let me know what you think.

comment:18 in reply to: ↑ 16 Changed 15 months ago by simonmar

Replying to dterei:

I think the backend may have bugs to do with this as it makes assumptions about word sizes... and so detection of various cases that need to be avoided aren't firing.

David, could you be more specific here? I'm not aware of any problems of this kind. If you have suspicions about lurking problems that might affect cross-compilation, let's get some tickets filed.

comment:19 Changed 15 months ago by dterei

Off the top of my head I think one issue will be that the LLVM backend has some code that is dependent on the endianness of the platform. It's just decided with an ifdef at compilation time...

Right now I'm handling the existing LLVM issues that Austin is filing but will look into cross compilation after that.

comment:20 Changed 15 months ago by thoughtpolice

OK, with the latest copy of GHC HEAD, LLVM 3.2 and GHC 7.6.1 as the bootstrap compiler on my Mac OS X machine, I no longer hit this during the build. (My build still fails later on - looks like it might be #7588 still.)

However, I speculate this is still a bug, because if you compile the attached test case with the compiler using the LLVM backend, it still trips the build even though it looks like I built the RTS successfully. So I think this should still be fixed.

comment:21 Changed 15 months ago by dterei

OK Cool Austin! I'll run your test cases for each bug and see. I'm glad at least we seem to be more on the same page now.

comment:22 Changed 15 months ago by dterei

OK yes, indeed the test fails and looking at the patch that is very expected. Applied patch works as expected.

comment:23 Changed 15 months ago by mad.one@…

commit 14c01e0966da2edf9b770651ce1a4ca6a206eb20

Author: Austin Seipp <mad.one@gmail.com>
Date:   Sun Jan 13 04:34:05 2013 +0000

    Ensure the LLVM codegen correctly handles literals in a branch. #7571
    
    We need to be sure that when generating code for literals, we properly narrow
    the type of the literal to i1. See Note [Literals and branch conditions] in the
    LlvmCodeGen.CodeGen module.
    
    This occurs rarely as the optimizer will remove conditional branches with
    literals, however we can get this situation occurring with hand written Cmm
    code.
    
    This fixes Trac #7571.
    
    Signed-off-by: David Terei <davidterei@gmail.com>

 compiler/llvmGen/LlvmCodeGen/CodeGen.hs |   86 +++++++++++++++++++++++++------
 1 files changed, 70 insertions(+), 16 deletions(-)

comment:24 Changed 15 months ago by dterei

  • Blocked By 7590 removed
  • Blocking 7590 added

Great, fixed! Thanks Austin, especially for the test case and improving the testsuite with the Cmm way. That is a big help going forward as a lot of bugs in LLVM have been due to rare cases that only appear in hand written cmm.

I changed the commit messages in the patches to be a little more informative, sorry for any hassle that causes for you.

comment:25 Changed 15 months ago by thoughtpolice

Killer, thanks David!

comment:26 Changed 15 months ago by dterei

  • Resolution set to fixed
  • Status changed from patch to closed

comment:27 Changed 15 months ago by dterei

  • Blocked By 7590 added
  • Blocking 7590 removed

comment:28 Changed 15 months ago by dterei

  • Blocked By 7590 removed
  • Blocking 7590 added

comment:29 Changed 15 months ago by dterei

  • Blocking 7589 removed

comment:30 Changed 15 months ago by davidterei@…

commit 1a703068117255592fb8d9d8d47a5d54640208d0

Author: David Terei <davidterei@gmail.com>
Date:   Wed Jan 23 00:38:43 2013 -0800

    Fix our handling of literals and types in LLVM (#7575).
    
    This bug was introduced in the recent fix for #7571, that extended some
    existing infastructure in the LLVM backend that handled the conflict
    between LLVM's return type from comparison operations (i1) and what GHC
    expects (word). By extending it to handle literals though, we forced all
    literals to be i1 or word, breaking other code.
    
    This patch resolves this breakage and handles #7571 still, cleaning up
    the code for both a little. The overall approach is not ideal but
    changing that is left for the future.

 compiler/llvmGen/LlvmCodeGen/CodeGen.hs |   64 ++++++++++++++-----------------
 1 files changed, 29 insertions(+), 35 deletions(-)

comment:31 Changed 15 months ago by dterei

  • Blocking 7589 added
Note: See TracTickets for help on using tickets.