Clean up opt and llc
ClosedPublic

Authored by angerman on Mar 15 2017, 9:01 PM.

Details

Summary

The LLVM backend shells out to LLVMs opt and llc tools. This clean up introduces
a shared data structure to carry the arguments we pass to each tool so that corresponding
flags are next to each other. It drops the hard coded data layouts in favor of using -mtriple
and have LLVM infer them. Furthermore we add clang as a proper tool, so we don't rely on
assuming that clang is called clang on the PATH when using clang as the assembler.
Finally this diff also changes the type of optLevel from Int to Word, as we do not have
negative optimization levels.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
angerman updated this revision to Diff 12652.May 21 2017, 5:41 AM
  • When a GNU eats an Apple
angerman updated this revision to Diff 12653.May 21 2017, 8:53 AM
  • Add llvm-targets
dfeuer added a subscriber: dfeuer.May 22 2017, 11:19 PM
dfeuer added inline comments.
compiler/main/DriverPipeline.hs
850

Is there a reason to calculate lengths here? Why not | not (null mcpu) and | not (null attrs)

874

So much nesting! Why does each string need its own list? Wouldn't this be the same?

intercalate "," $ mattr ++
   [ ["+sse42" | ...]
   , ["+sse2" | ...]
   , ...]
angerman updated this revision to Diff 12682.May 22 2017, 11:29 PM
  • bump allocation stat
angerman marked 2 inline comments as done.May 23 2017, 1:01 AM
angerman added inline comments.
compiler/main/DriverPipeline.hs
850

good idea!

874

I guess we can get rid of this. If someone ends up needing more that a single attribute for a flag, we'll either need to revert to this, or use words.

angerman updated this revision to Diff 12684.May 23 2017, 1:01 AM
angerman marked 2 inline comments as done.
  • Cleanup
angerman planned changes to this revision.May 28 2017, 7:09 PM

Something in our opt -> llc -> mangler -> as pipeline results in bad relocations being generated on aarch64.

kavon added a comment.May 29 2017, 5:25 AM

Something in our opt -> llc -> mangler -> as pipeline results in bad relocations being generated on aarch64.

If the bug isn't caused by this patch, we should create a new phab review for it.

In D3352#102743, @kavon wrote:

Something in our opt -> llc -> mangler -> as pipeline results in bad relocations being generated on aarch64.

If the bug isn't caused by this patch, we should create a new phab review for it.

This is precisely the issue, I did get around by using clang :-/. Dropping the mangler and assembler and letting llc
produce the obj directly seems to produce proper code. I however do not exactly know what the reason here is.

Also the aarch64 stage2 compiler segfaults for some yet unknown reason.

Just to add to that. I'm really keen in figuring out what is going wrong here, I believe it's something with -fPIC and some small memory model llvm infers.

angerman updated this revision to Diff 12716.May 29 2017, 8:23 AM
  • minor adjustments, validation pending.
This revision is now accepted and ready to land.May 29 2017, 8:23 AM
angerman updated this revision to Diff 12717.May 29 2017, 9:18 PM
  • drop mtriple and data-layout, as they are now hardcoded in the ir file.
  • bring back rmodel.
angerman updated this revision to Diff 12724.May 30 2017, 8:26 PM
  • always PIC
rwbarton requested changes to this revision.May 31 2017, 8:04 AM

We should not just force PIC on all LLVM users.

This revision now requires changes to proceed.May 31 2017, 8:04 AM

We should not just force PIC on all LLVM users.

No we should definitely not. We might want to control this via the llvm-targets file. The slight bit annoying part is, that if this the stage1 compiler doesn't set this properly
the libraries that ship with the cross compiler are broken and no matter what flags one passes to the cross compiler afterwards will change the libraries. This has been a
rather annoying bug to track down, as the build systems change tracking doesn't catch it.

angerman planned changes to this revision.May 31 2017, 8:54 AM

I agree, PIC is not free and we shouldn't assume that the user needs it. I'm curious to know why this was necessary.

I agree, PIC is not free and we shouldn't assume that the user needs it. I'm curious to know why this was necessary.

-- Relocation models
-- we default to always "pic".
--
-- without it we run into the following two issues:
-- - unresolvable R_AARCH64_ADR_PREL_PG_HI21
--   relocation against symbol `free@@LIBC'
-- - creating a DT_TEXTREL in a shared object.
--

;-)

This certainly warrants some deeper investigation. The first one supposedly has something to do with a small memory model
that llvm is assuming. I'm not much a fan of PIC, unless needed, I do have a few ideas as to what to experiment with. As of
right now, and using this diff myself, producing PIC is the variant that works in all linking scenarios I have encountered so far.
As this has really only been a major issue with aarch64/android so far, I believe getting the default flag from the llvm-targets
file, might be a good option, as it would ensure that the libraries are built accordingly as well.

angerman updated this revision to Diff 13033.Jul 4 2017, 10:55 PM
angerman edited edge metadata.
  • rebase onto master
This revision is now accepted and ready to land.Jul 4 2017, 10:55 PM
angerman updated this revision to Diff 13086.Jul 9 2017, 9:11 PM
  • rebase + drop always PIC
This revision now requires review to proceed.Jul 9 2017, 9:11 PM
angerman updated this revision to Diff 13092.Jul 10 2017, 7:56 PM
  • drop Platform import
angerman updated this revision to Diff 13123.Jul 11 2017, 8:02 PM
  • rebase & drop unused local bind dl
angerman updated this revision to Diff 13131.Jul 12 2017, 2:57 AM
  • rebase onto fixed master

What is the status of this?

angerman planned changes to this revision.Aug 31 2017, 10:19 AM

I'll rebase this. I've been using this for quite some time without any issues.

angerman updated this revision to Diff 13734.Sep 5 2017, 2:00 AM
  • rebase

The most recent rebase ran into conflicts with D3589. Specifically it collided with setting rmodel = "pic" in case of positionIndependent dflags. See also the picCCOpts dflags.

angerman updated this revision to Diff 13735.Sep 5 2017, 2:07 AM
  • Adds the pic relocation model back
bgamari requested changes to this revision.Sep 5 2017, 5:48 AM

This looks quite reasonable to me. Just a few quick comments.

compiler/llvmGen/LlvmCodeGen.hs
92

I suspect we should have a proper error message here.

compiler/main/DriverPipeline.hs
862

Ahh, is this what you had meant by the AVX mangler hack no longer being necessary?

876

What does this do? Will this some day turn into a dynflag?

1585

Non-native?

testsuite/tests/perf/compiler/all.T
1076

Perhaps this should be flipped around?

This revision now requires changes to proceed.Sep 5 2017, 5:48 AM
angerman marked 2 inline comments as done.Sep 5 2017, 6:37 AM
angerman added inline comments.
compiler/main/DriverPipeline.hs
862

I'm not sure :(

876

I hope so. The fastLlvmPipeline make llc emit object files directly. Instead of going through the mangler, splitter, assembler, ... by providing llc with -filetype=obj.

angerman updated this revision to Diff 13751.Sep 5 2017, 6:39 AM
angerman edited edge metadata.
angerman marked 2 inline comments as done.
  • Fixes
angerman updated this revision to Diff 13752.Sep 5 2017, 7:10 AM
  • drop llvm-targets
angerman updated this revision to Diff 13759.Sep 6 2017, 2:21 AM
  • -fast-llvm Flag.
bgamari added inline comments.Sep 6 2017, 10:30 AM
compiler/main/DriverPipeline.hs
1513

Perhaps this logic should be documented in the users guide?

This revision was automatically updated to reflect the committed changes.