Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#10170 closed bug (fixed)

Find versioned versions of LLVM tools

Reported by: erikd Owned by: erikd
Priority: normal Milestone: 8.0.1
Component: Compiler (LLVM) Version: 7.11
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D745
Wiki Page:

Description

Since it has become more widely know that the LLVM developers often make breaking changes to the IR language between releases, some Linu distributions (eg Debain and hence Ubuntu) have started doing multi-version LLVM installs.

For instance on Debian, the llvm-3.6 programs are installed in /usr/lib/llvm-3.6/bin as llc and opt.

I propose that the configure.ac and aclocal.m4 configuration goop be modified to look for llc and opt in these versioned /usr/lib/llvm-X.Y locations. I'm also willing to do the hard yards on implementing this if people think its a good idea.

This can be seen as a step along the way to more stringent LLVM versioning requirements.

Change History (18)

comment:1 Changed 3 years ago by thomie

This should already work. If it doesn't, than that's indeed a bug. See commit 1dfab7a8ace5f09f00f8fb695932b4324e88b822

Author: Thomas Miedema <thomasmiedema@gmail.com>
Date:   Mon Mar 2 11:09:33 2015 -0600

    Fix detection of llvm-x.x
    
    Summary:
    Four bug fixes and a little refactoring.
    * `find -perm \mode` should be `find -perm /mode` (#9697)
    
    * `find -regex '$3' should be `find -regex "$3"` (#7661)
    
    From `man sh` on my system (Ubuntu 14.04):
    "Enclosing characters in single quotes preserves the literal meaning of all
    the characters ..."
    
    * LlcCmd and OptCmd should be passed to ghc, using `-pgmlo` and `-pgmlc`, for
      detection of #9439.
    
    * -pgmlo and -pgmlc were undocumented because of an xml tag misplacement.
    
    Test Plan:
    The aclocal.m4 macro has seen about 10 iterations since its inception. Without a
    testsuite, I can't guarantee this version is bug free either. It's all pretty
    frustrating.
    
    Reviewers: bgamari, austin
    
    Reviewed By: austin
    
    Subscribers: thomie
    
    Differential Revision: https://phabricator.haskell.org/D683
    
    GHC Trac Issues: #9697, #7661, #9439

comment:2 Changed 3 years ago by erikd

@thomie It doesn't work now, because on my machine it picks /usr/bin/llc (which is actually a symlink to /usr/lib/llvm-3.5/bin/llc) before it picks /usr/lib/llvm-3.6/bin/llc. I can't remove llvm-3.5 because I have other tools that rely on it.

If people think its a good idea, I would make the configure script check for a specific version of the llvm tools (currently 3.6 because thats all that seems to work currently). With this versioned check, when we move to another version of LLVM there should be one single change to configure.ac to make it check for the new version.

comment:3 Changed 3 years ago by thomie

Sounds good, to me at least. Hopefully it allows you to delete that FIND_LLVM macro.

comment:4 Changed 3 years ago by rwbarton

On my Debian system there is also /usr/bin/llc-3.x which is a symlink to /usr/lib/llvm-3.x/bin/llc. If GHC requires LLVM 3.6, for instance, wouldn't it be best to look for llc-3.6 in the $PATH, rather than specifically /usr/lib/llvm-3.6/bin/llc? Then it would also work if the user installed llc-3.6 other than through their package manager (e.g., in ~/bin).

Maybe this is what you were suggesting in comment:2, not sure.

comment:5 Changed 3 years ago by erikd

@rwbarton Yes, it should look for a versioned /usr/lib/llc-X.Y but also accept /usr/lib/llvm-3.6/bin/llc (I'm pretty sure the versioned symlinks in /usr/bin/ are managed by the Debian alternatives system and therefore optional). More importantly configure should fail if whatever version of llc it finds isn't the currently blessed version.

@thomie Unfortunately, I think that FIND_LLVM macro will need to grow, not disappear.

Last edited 3 years ago by erikd (previous) (diff)

comment:6 Changed 3 years ago by erikd

Nope, it turns out that on Debian, the /usr/bin/llc-3.6 symlink is managed by the llvm-3.6 package. Its the /usr/bin/llc symlink that is managed by the Debian alternatives system.

comment:7 Changed 3 years ago by rwbarton

More importantly configure should fail if whatever version of llc it finds isn't the currently blessed version.

Right.

Yes, it should look for a versioned /usr/lib/llc-X.Y but also accept /usr/lib/llvm-3.6/bin/llc

Let me rephrase what I meant. Hard-coding knowledge of the directory /usr/lib/llvm-3.6/bin into ghc's autoconf is wrong because (in my estimation) the fact that llc-3.6 is a symlink to /usr/lib/llvm-3.6/bin/llc is an implementation detail of the Debian llvm-3.6 package and the executable may be (and is) located elsewhere on other distributions. The public interface of llvm-3.6 (on Debian and hopefully on other distributions) is that it provides llc-3.6 on the standard $PATH.

comment:8 Changed 3 years ago by thomie

I think that FIND_LLVM macro will need to grow, not disappear.

In configure.ac, can't you just replace:

FIND_LLVM_PROG([LLC], [llc], [llc])

With:

FP_ARG_WITH_PATH_GNU_PROG_OPTIONAL_NOTARGET([LLC], [llc], [llc-3.6])

comment:9 Changed 3 years ago by nomeata

I thought the plan was to ship llvm with ghc; wouldn’t that make this obsolete?

(Distro packagers can still explicitly tell ghc to use a system installed llvm of the right version using --with-llc etc.)

comment:10 Changed 3 years ago by thomie

We need llvm 3.6 now, for running the full testsuite.

comment:11 Changed 3 years ago by erikd

@thomie That would work for some cases, but imo, if llc-3.6 does not exist, the configure script should still check to see if llc is the required version.

@rwbarton I agree about not looking for /usr/lib/llvm-3.6/bin/llc. That was a silly idea.

@nomeata That may be true, but we still need to fix this for people compiling from git.

comment:12 Changed 3 years ago by erikd

I've been doing some hacking on this and here is my current solution (for now X.Y is 3.6):

  • The configure script should check for llc-X.Y and opt-X.Y *before* it checks for plain llc and opt.
  • Only check for plain llc and opt if the explicitly versioned ones are not found.
  • If it finds plain llc and opt check that the version is X.Y.

The above currently works, but causes other problems in this check:

checking whether bootstrap compiler is affected by bug 9439... You are using a new version of LLVM that hasn't been tested yet!
We will try though...
/usr/bin/opt-3.6: /tmp/ghc12163_0/ghc12163_2.ll:7:6: error: unexpected type in metadata definition
!0 = metadata !{metadata !"top", i8* null}
     ^
failed to compile

The problem here is that this checks the bootstrap compiler bug using the detected LlcCmd (which now *must* be llc-3.6) which doesn't support he LLVM IR syntax above. I think is in-correct.

The bootstrap compiler should already have the location of the llc and opt it was compiled for in its settings file (these have been present in that file since ghc 7.6 but they are not present in eg 7.4.2). Its the llc version in the settings file which should be checked for bug #9439, not the one detected by the configure script.

comment:13 Changed 3 years ago by thomie

That's easy to fix. Revert this change I made in 1dfab7a8ace5f09f00f8fb695932b4324e88b822:

\--- a/configure.ac
+++ b/configure.ac
@@ -508,7 +508,7 @@ then
     echo "main = putStrLn \"%function\"" > conftestghc.hs
 
     # Check whether LLVM backend is default for this platform
-    "${WithGhc}" conftestghc.hs 2>&1 >/dev/null
+    "${WithGhc}" -pgmlc="${LlcCmd}" -pgmlo="${OptCmd}" conftestghc.hs 2>&1 >/dev/null
     res=`./conftestghc`
     if test "x$res" = "x%object"
     then
@@ -525,7 +525,7 @@ then
 
     # -fllvm is not the default, but set a flag so the Makefile can check
     # -for it in the build flags later on
-    "${WithGhc}" -fforce-recomp -fllvm conftestghc.hs 2>&1 >/dev/null
+    "${WithGhc}" -fforce-recomp -pgmlc="${LlcCmd}" -pgmlo="${OptCmd}" -fllvm conftestghc.hs 2>&1 >/dev/null
     if test $? = 0
     then
         res=`./conftestghc`

comment:14 Changed 3 years ago by erikd

Differential Rev(s): D745

comment:15 Changed 3 years ago by rwbarton

Differential Rev(s): D745Phab:D745

comment:16 Changed 3 years ago by Erik de Castro Lopo <erikd@…>

In 42448e3757f25735a0a5b5e2b7ee456b5e8b0039/ghc:

Do version specific detection of LLVM tools (#10170).

The LLVM developers seem to make breaking changes in the LLVM IR
language between major releases. As a consumer of the LLVM tools
GHC now needs to be locked more tightly to a single version of
the LLVM tools.

GHC HEAD currently only supports LLVM version 3.6. This commit
changes the configure script to look for `llc-3.6` and `opt-3.6`
before looking for `llc` and `opt`. If the former are not found,
but the later are, check that they actually are version 3.6.

At the same time, when detecting known problems with the LLVM
tools (ie #9439) test for it using the versions of the LLVM tools
retrieved from the bootstrap compiler's settings file.

Test Plan: Manual testing.

Reviewers: thomie, rwbarton, nomeata, austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D745

GHC Trac Issues: #10170

comment:17 Changed 3 years ago by erikd

Resolution: fixed
Status: newclosed

Since this patch is all about llvm-3.6, this is a 7.11+ specific patch and hence does not need to be backported to 7.10.

comment:18 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

Note: See TracTickets for help on using tickets.