Page MenuHomePhabricator

[GCC] Attribute ifunc support in llvm
ClosedPublic

Authored by DmitryPolukhin on Dec 15 2015, 6:36 AM.

Details

Summary

This patch add support for GCC attribute((ifunc("resolver"))) for targets that use ELF as object file format. In general ifunc is a special kind of function alias with type @gnu_indirect_function. Patch for Clang http://reviews.llvm.org/D15524

Diff Detail

Repository
rL LLVM

Event Timeline

DmitryPolukhin retitled this revision from to [GCC] Attribute ifunc support in llvm.
DmitryPolukhin updated this object.
DmitryPolukhin added a subscriber: llvm-commits.
rafael added a subscriber: rafael.
rafael added inline comments.
include/llvm/IR/GlobalAlias.h
90 ↗(On Diff #42846)

Don't repeat name in comments.

Fixed comment.

Since this modifies the IR, please start with an RFC on llvm-dev.
(I just did a search for "ifunc"... sorry if you already posted
one and I'm just incapable of using search correctly.)

I'd be interested, in particular, to hear whether you considered
adding a separate construct rather than piggy-backing on
GlobalAlias, and if so, why you rejected that alternative. It
seems like, semantically, ifuncs are very different from aliases.
A straw-man counter-proposal might be:

  • Move the relevant guts of GlobalAlias into a base class, maybe called GlobalIndirectSymbol (bike shed that).
  • Add a new subclass called GlobalIFunc.
  • Use ifunc *instead* of alias in textual IR (making it a separate construct, rather than an attribute).

The main benefit? Current optimizations/etc. may assume that
isa<GlobalAlias>(GV) implies that GV is a global alias. It
seems clearer to use the type hierarchy to differentiate. It'll
probably be cleaner for new code, as well.

Regardless, please take it to an llvm-dev RFC so we can get more
eyes on it. (Although given the time of year, you may not get
the usual number of responses over the next couple of weeks...)

echristo edited edge metadata.Dec 17 2015, 7:43 PM
echristo added a subscriber: echristo.

FWIW both I and John had the feedback on GlobalAlias on the cfe-commits
thread. :)

DmitryPolukhin marked an inline comment as done.Dec 18 2015, 1:17 AM

Thank you for pointing out the direction. I'll start working on RFC with different approaches how ifunc could be represented in IR.

Just for the sake of completeness, there is a way to avoid IR extension - it is possible to emit global asm statement like __asm__ (".type resolver_alias_name, @gnu_indirect_function") and avoid IR extension but I didn't seriously consider this path.

DmitryPolukhin edited edge metadata.

Work in progress patch that shows amount of changes required to introduce GloablIFunc and put it into separate list in module.

Clang part also needs to be update but it depends on LLVM part so it makes sense to reach at least some consensus on LLVM part first.

rafael added inline comments.Dec 24 2015, 7:20 AM
include/llvm-c/Core.h
1890 ↗(On Diff #43599)

I am not sure about adding a C api right away. Maybe wait a bit once the patch is in?

include/llvm/IR/GlobalIndirectSymbol.h
63 ↗(On Diff #43599)

Is this meaningful for ifunc?

IMHO it is a design mistake for GlobalAlias to take an arbitrary expression. If ifunc can take just a GlobalValue that would be nice.

include/llvm/IR/Module.h
591 ↗(On Diff #43599)

Is there real value in these @{}?

593 ↗(On Diff #43599)

clang-format

lib/AsmParser/LLParser.cpp
719 ↗(On Diff #43599)

Any type restrictions when it is not an alias?

lib/AsmParser/LLParser.h
274 ↗(On Diff #43599)

Start functions with lowercase.

lib/Bitcode/Writer/BitcodeWriter.cpp
781 ↗(On Diff #43599)

You are missing the address space in the comment.

DmitryPolukhin marked 4 inline comments as done.

Still work in progress, fixed most of comments.

New patch uploaded.

include/llvm-c/Core.h
1890 ↗(On Diff #43599)

I added it just for completeness. I can remove it before commit if needed. I'm worry that it will be easy to forget if I remove it now.

include/llvm/IR/GlobalIndirectSymbol.h
63 ↗(On Diff #43599)

It makes sense to me.

include/llvm/IR/Module.h
591 ↗(On Diff #43599)

I don't see them in generated doxygen so it seems useless. I put them just to follow style of surrounding code. I'll remove them if there a consensus here.

593 ↗(On Diff #43599)

Same here, such formatting is used in the whole file very consistent so I decided to follow local style. Please let me know if you really want to reformat only new block of code differently.

lib/AsmParser/LLParser.cpp
719 ↗(On Diff #43599)

At least we could check that it is a function. Checking the function return type is trickier because GCC allows any type even void same as for the parameters of the function.

Clang part was updated to use new representation so I think now patch is ready for review, please take a look.

Friendly ping, please review this patch.

Clang part of ifunc support was approved for commit but llvm part needs to be committed first.

vsk added a subscriber: vsk.Jan 11 2016, 9:32 AM

Please add a short test to test/Bitcode/compatibility.ll.

  • added test case in test/Bitcode/compatibility.ll
  • added test test/Assembler/ifunc-use-list-order.ll
  • fixed ordering problem in bitcode writer
  • fix for alias on ifunc
  • fixed nit in LangRef

PTAL

DmitryPolukhin edited reviewers, added: chandlerc; removed: joe.abbey.Jan 18 2016, 6:02 AM

Friendly ping, please take a look!

Please let me know if in general things look fine but this patch needs to be splatted into smaller parts to simplify review as Duncan suggested.

Friendly ping, please take a look!

Friendly ping. As a user I'm really looking forward to this feature. Thanks DmitryPolukhin for writing it.

asl added a subscriber: asl.Mar 18 2016, 8:36 AM
asl added inline comments.
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1126 ↗(On Diff #44637)

Why alias? Shouldn't this be llvm_unreachable?

A couple of inline comments for the next patch, but a quick top level comment:

Since we agree that the GlobalIndirectFunc hierarchy is the right way to do this, can you split the patch in two and add that layer of indirection first and then we can add ifunc later to make it easier to see what each side of things is doing?

Also, thank you very much for all of your work and patience here. I really appreciate it.

Thanks!

-eric

include/llvm-c/Core.h
1790 ↗(On Diff #44637)

We typically add them on demand. I'd rather leave it off for now and we can add it later if it's necessary.

include/llvm/IR/Module.h
582 ↗(On Diff #44637)

No, consistency is fine here, thanks.

Eric, that you for looking to the code! I'll split the patch and send first part for review ASAP.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1126 ↗(On Diff #44637)

Local linkage is possible here but I'll update text to reflect that ifunc could be here too.

Extracted GlobalIndirectSymbol creation to separate patch http://reviews.llvm.org/D18433

This patch will be rebased when D18433 is committed to continue ifunc specific review.

  • rebease on top of GlobalIndirectSymbol committed to LLVM
  • removed C interface for ifunc, it seems that it is not required for now

Highlighted a couple of things that could be pulled out into an incremental patch (and you can pull out similar Alias->IndirectSymbol changes).

Thanks!

-eric

include/llvm/CodeGen/AsmPrinter.h
550–552 ↗(On Diff #52205)

This isn't part of the previous patch?

lib/AsmParser/LLParser.h
278–282 ↗(On Diff #52205)

Ditto.

DmitryPolukhin added inline comments.Apr 1 2016, 6:55 AM
include/llvm/CodeGen/AsmPrinter.h
550–552 ↗(On Diff #52205)

In previous patch I included things that have sense without ifunc and functions that won't be updated in ifunc patch. For printer and bitcode write the same functions will have to be updated twice so it won't reduce this patch so significantly. But OK, I'll extract as much as I can.

echristo added inline comments.Apr 2 2016, 12:43 AM
include/llvm/CodeGen/AsmPrinter.h
550–552 ↗(On Diff #52205)

Awesome. Thank you very much :)

Rebased on top of D18754

Eric, thank you a lot for fast review previous parts and please take a look to the remaining changes.

echristo accepted this revision.Apr 5 2016, 3:59 PM
echristo edited edge metadata.

LGTM. Since Duncan has been looking at these as well you might want to wait for an ACK from him as well.

Thanks a ton for doing all of this. :)

-eric

This revision is now accepted and ready to land.Apr 5 2016, 3:59 PM

I have a bunch of nitpicks inline below; once you resolve those, LGTM!

DmitryPolukhin edited edge metadata.
  • fixed comments
This revision was automatically updated to reflect the committed changes.