<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Eric and Duncan than you a lot for the review!</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Wed, Apr 6, 2016 at 5:53 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> --- lib/IR/Module.cpp<br>
> +++ lib/IR/Module.cpp<br>
> @@ -250,6 +252,13 @@<br>
> return dyn_cast_or_null<GlobalAlias>(getNamedValue(Name));<br>
> }<br>
><br>
> +// getNamedIFunc - Look up the specified global in the module symbol table.<br>
> +// If it does not exist, return null.<br>
> +//<br>
<br>
Please put this comment in the header instead of the source file, and<br>
don't duplicate the identifier ("getNamedIFunc").<br>
<br>
(If you're up to it, ideally you'd make the same transformation for the<br>
existing function comments in this file as a prep commit.)<br></blockquote><div><br></div><div>The header already has similar comment so it is kind of duplication therefore I just remove it. I'll update existing functions in separate patch later, need careful comparison of the comments in .cpp and .h file.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> --- lib/IR/Globals.cpp<br>
> +++ lib/IR/Globals.cpp<br>
> @@ -145,6 +145,8 @@<br>
> return const_cast<GlobalObject *>(GO)->getComdat();<br>
> return nullptr;<br>
> }<br>
> + if (isa<GlobalIFunc>(this))<br>
> + return nullptr;<br>
<br>
Should this try to look through the GlobalIFunc and return the same<br>
comdat? (I honestly don't know; I don't know much about comdats.)<br>
<br>
- If so, please add that (maybe change the above code to use<br>
GlobalIndirectSymbol?).<br>
<br>
- If not, please add a comment explaining why it's appropriate for<br>
GlobalAlias and not GlobalIFunc.<br></blockquote><div><br></div><div>Alias is just another name for the same thing. As for ifunc and its resolver they are completely separate things. Added comment.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> --- lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> +++ lib/Bitcode/Writer/BitcodeWriter.cpp<br>
> @@ -809,6 +809,19 @@<br>
> Vals.clear();<br>
> }<br>
><br>
> + // Emit the ifunc information.<br>
<span>> + for (const GlobalIFunc &I : M->ifuncs()) {<br>
</span>> + // IFUNC: [ifunc type, address space, resolver val#, linkage, visibility]<br>
> + Vals.push_back(VE.getTypeID(I.getValueType()));<br>
> + Vals.push_back(I.getType()->getAddressSpace());<br>
> + Vals.push_back(VE.getValueID(I.getResolver()));<br>
> + Vals.push_back(getEncodedLinkage(I));<br>
> + Vals.push_back(getEncodedVisibility(I));<br>
> + unsigned AbbrevToUse = 0;<br>
> + Stream.EmitRecord(bitc::MODULE_CODE_IFUNC, Vals, AbbrevToUse);<br>
<br>
This code is equivalent to the last two lines and a little cleaner:<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
Stream.EmitRecord(bitc::MODULE_CODE_IFUNC, Vals);<br></blockquote><div><br></div><div>Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> --- lib/Bitcode/Reader/BitcodeReader.cpp<br>
> +++ lib/Bitcode/Reader/BitcodeReader.cpp<br>
> @@ -3658,8 +3658,10 @@<br>
> }<br>
> // ALIAS: [alias type, addrspace, aliasee val#, linkage]<br>
> // ALIAS: [alias type, addrspace, aliasee val#, linkage, visibility, dllstorageclass]<br>
> + // IFUNC: [alias type, addrspace, aliasee val#, linkage, visibility, dllstorageclass]<br>
> case bitc::MODULE_CODE_ALIAS:<br>
> - case bitc::MODULE_CODE_ALIAS_OLD: {<br>
> + case bitc::MODULE_CODE_ALIAS_OLD:<br>
> + case bitc::MODULE_CODE_IFUNC: {<br>
<br>
Diff would be one line shorter if you put MODULE_CODE_IFUNC *before*<br>
MODULE_CODE_ALIAS ;).<br></blockquote><div><br></div><div>Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> @@ -3687,7 +3689,8 @@<br>
> NewGA = GlobalAlias::create(<br>
> Ty, AddrSpace, getDecodedLinkage(Linkage), "", TheModule);<br>
> else<br>
> - llvm_unreachable("Not an alias!");<br>
> + NewGA = GlobalIFunc::create(<br>
> + Ty, AddrSpace, getDecodedLinkage(Linkage), "", nullptr, TheModule);<br>
<br>
Have you run this through clang-format? I think clang-format would<br>
indent this an extra two spaces. (Not that you have to abide by<br>
clang-format, just made me wonder...)<br></blockquote><div><br></div><div>I re-run clang format on this particular code fragment and now it formatted it completely different. Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> --- /dev/null<br>
> +++ include/llvm/IR/GlobalIFunc.h<br>
> @@ -0,0 +1,74 @@<br>
> +//===-------- llvm/GlobalIFunc.h - GlobalIFunc class ------------*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +///<br>
> +/// \brief<br>
> +/// This file contains the declaration of the GlobalIFunc class, which<br>
> +/// represents a single indirect function in the IR. Indirect function uses<br>
> +/// ELF symbol type extension to mark that the address of a declaration should<br>
> +/// be resolved at runtime by calling a resolver function.<br>
> +///<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#ifndef LLVM_IR_GLOBALIFUNC_H<br>
> +#define LLVM_IR_GLOBALIFUNC_H<br>
> +<br>
> +#include "llvm/ADT/Twine.h"<br>
> +#include "llvm/ADT/ilist_node.h"<br>
> +#include "llvm/IR/GlobalIndirectSymbol.h"<br>
> +<br>
> +namespace llvm {<br>
> +<br>
> +class Module;<br>
> +template <typename ValueSubClass> class SymbolTableListTraits;<br>
> +<br>
<br>
A comment for this class would be nice.<br></blockquote><div><br></div><div>Done.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> +class GlobalIFunc : public GlobalIndirectSymbol,<br>
<br>
final?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> + public ilist_node<GlobalIFunc> {<br>
> + friend class SymbolTableListTraits<GlobalIFunc>;<br>
> + void operator=(const GlobalIFunc &) = delete;<br>
> + GlobalIFunc(const GlobalIFunc &) = delete;<br>
> +<br>
> + void setParent(Module *parent);<br>
> +<br>
> + GlobalIFunc(Type *Ty, unsigned AddressSpace, LinkageTypes Linkage,<br>
> + const Twine &Name, Constant *Resolver, Module *Parent);<br>
> +<br>
> +public:<br>
> + /// If a parent module is specified, the ifunc is automatically inserted into<br>
> + /// the end of the specified module's ifunc list.<br>
> + static GlobalIFunc *create(Type *Ty, unsigned AddressSpace,<br>
> + LinkageTypes Linkage, const Twine &Name,<br>
> + Constant *Resolver, Module *Parent);<br>
> +<br>
> + /// This method unlinks 'this' from the containing module, but does not<br>
> + /// delete it.<br>
> + void removeFromParent() override;<br>
<br>
final?<br></blockquote><div><br></div><div>Done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">> +<br>
> + /// This method unlinks 'this' from the containing module and deletes it.<br>
> + void eraseFromParent() override;<br>
<br>
final?<br></blockquote><div><br></div><div>Done.</div></div></div></div>