Opened 4 years ago

Closed 4 years ago

#10394 closed bug (fixed)

LLVM mangler doesn't mangle AVX instructions

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

Description

The LLVM mangler does not currently transform AVX instructions on x86-64 platforms, due to a missing #include. Also, it is significantly more complicated than necessary, due to the file into sections (not needed anymore), and is sensitive to the details of the whitespace in the assembly.

I have attached a modified mangler that I believe to be simpler and more robust. I have not tested it, though, as I do not have a recent enough version of LLVM on my machine.

I am marking this as `Runtime crash' because that is what would happen if the unchanged AVX instructions made their way into the executable.

Attachments (1)

LlvmMangler.hs (4.2 KB) - added by dobenour 4 years ago.
New LLVM mangler

Download all attachments as: .zip

Change History (9)

Changed 4 years ago by dobenour

Attachment: LlvmMangler.hs added

New LLVM mangler

comment:1 Changed 4 years ago by rwbarton

Cc: bgamari added

In fact the CPP around the AVX rewriting stuff was broken anyways (should have been #ifdef REWRITE_AVX not #if REWRITE_AVX) so even if x86_64_TARGET_ARCH was set, the result would have been a CPP error. It looks like this never actually worked.

Makes me wonder if we even need the AVX rewriting nowadays; or maybe we've told LLVM not to use these instructions, and could now allow it to; or maybe we can tell it not to assume the stack is 32-byte aligned.

Your rewrite looks generally sensible, though I haven't examined it closely. cc'ing Ben since I think he did the conversion to use prefix data for tables-next-to-code.

comment:2 Changed 4 years ago by thomie

Status: newpatch

Paging bgamari.

comment:3 Changed 4 years ago by bgamari

Ahh, sorry, this somehow slipped through the cracks. I'll have a look first thing tomorrow.

comment:4 Changed 4 years ago by bgamari

Differential Rev(s): Phab:D1034

I've opened Phab:D1034 with this. dobenour, you should feel free to commandeer this Diff so it has the proper attribution. I'll review the patch itself on Phab.

Indeed it would be nice if we could eliminate this mangling altogether. I'll add it to my queue although that shouldn't discourage others from looking at this.

comment:5 Changed 4 years ago by Ben Gamari <ben@…>

In 600b153a/ghc:

llvmGen: Rework LLVM mangler

The LLVM mangler does not currently transform AVX instructions on x86-64
platforms, due to a missing #include. Also, it is significantly more
complicated than necessary, due to the file into sections (not needed
anymore), and is sensitive to the details of the whitespace in the
assembly.

Author: dobenour

Test Plan: Validation on x86-64, x86-32, and ARM

Reviewers: austin

Subscribers: thomie, bgamari, rwbarton

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

GHC Trac Issues: #10394

comment:6 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:7 Changed 4 years ago by thomie

What is the status here. Fixed?

Edit: answering my own question. No, see comment:4:

Indeed it would be nice if we could eliminate this mangling altogether.

Last edited 4 years ago by thomie (previous) (diff)

comment:8 Changed 4 years ago by bgamari

Resolution: fixed
Status: patchclosed

This particular issue can be closed.

The more general problem of the LLVM mangler we'll track as #11138.

Note: See TracTickets for help on using tickets.