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
Details
Diff Detail
Event Timeline
include/llvm/IR/GlobalAlias.h | ||
---|---|---|
90 ↗ | (On Diff #42846) | Don't repeat name in comments. |
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...)
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.
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.
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 | 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 | ||
589 | Is there real value in these @{}? | |
591 | clang-format | |
lib/AsmParser/LLParser.cpp | ||
757 | 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 | ||
814 | You are missing the address space in the comment. |
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 | It makes sense to me. | |
include/llvm/IR/Module.h | ||
589 | 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. | |
591 | 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 | ||
757 | 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.
- 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
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. As a user I'm really looking forward to this feature. Thanks DmitryPolukhin for writing it.
lib/CodeGen/AsmPrinter/AsmPrinter.cpp | ||
---|---|---|
1072 | 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 | ||
591 | 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 | ||
---|---|---|
1072 | 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. |
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. |
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.
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
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.