Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 Test Case:
Blocked By: #7573 Blocking: #7588, #7589, #7590
Related Tickets: #7574, #7575 Differential Revisions:

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 2 years ago.
Fix reference to 'Note' in commit.
0001-Test-for-Trac-7571.patch (1.3 KB) - added by thoughtpolice 2 years ago.
Patch for testsuite.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 2 years 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 <[email protected]>
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 <[email protected]>

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

Changed 2 years ago by thoughtpolice

Fix reference to 'Note' in commit.

comment:2 Changed 2 years ago by thoughtpolice

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

comment:3 Changed 2 years ago by thoughtpolice

I will have a test for this shortly I hope.

Changed 2 years ago by thoughtpolice

Patch for testsuite.

comment:4 Changed 2 years 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 <[email protected]>
Date:   Sun Jan 13 03:51:36 2013 -0600

    Test for Trac #7571.
    
    Signed-off-by: Austin Seipp <[email protected]>

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

comment:5 Changed 2 years ago by thoughtpolice

See also #7574.

comment:6 Changed 2 years ago by thoughtpolice

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

comment:7 Changed 2 years ago by thoughtpolice

comment:8 Changed 2 years 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 2 years ago by thoughtpolice

comment:10 Changed 2 years 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 2 years 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 2 years 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 2 years ago by thoughtpolice

  • Blocking 7588 added

comment:14 Changed 2 years ago by dterei

  • Blocking 7589 added

comment:15 Changed 2 years ago by dterei

  • Blocked By 7590 added

comment:16 follow-up: Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by mad.one@…

commit 14c01e0966da2edf9b770651ce1a4ca6a206eb20

Author: Austin Seipp <[email protected]>
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 <[email protected]>

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

comment:24 Changed 2 years 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 2 years ago by thoughtpolice

Killer, thanks David!

comment:26 Changed 2 years ago by dterei

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

comment:27 Changed 2 years ago by dterei

  • Blocked By 7590 added
  • Blocking 7590 removed

comment:28 Changed 2 years ago by dterei

  • Blocked By 7590 removed
  • Blocking 7590 added

comment:29 Changed 2 years ago by dterei

  • Blocking 7589 removed

comment:30 Changed 2 years ago by davidterei@…

commit 1a703068117255592fb8d9d8d47a5d54640208d0

Author: David Terei <[email protected]>
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 2 years ago by dterei

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