[PATCH] D37581: Implement pagerando wrapper functions to initialize POT register

[Nick Lewycky via Phabricator via llvm-commits]

nlewycky added inline comments.


================
Comment at: include/llvm/IR/Attributes.td:182-186
+/// Function is a Pagerando wrapper.
+def PagerandoWrapper : EnumAttr<"pagerando_wrapper">;
+
+/// Function is placed into a Pagerando bin.
+def PagerandoBinned : EnumAttr<"pagerando_binned">;
----------------
I don't see a matching LangRef change.

I don't see a matching Verifier change. These are only applicable to functions (as opposed to return values or arguments)? Are these two attributes mutually exclusive?


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:77
+      || F.hasComdat()  // TODO: Support COMDAT
+      || isa<UnreachableInst>(F.getEntryBlock().getTerminator());
+      // Above condition is different from F.doesNotReturn(), which we do not
----------------
rinon wrote:
> vlad.tsyrklevich wrote:
> > Could you explain this condition?
> The last condition handles functions that consist of a single unreachable instruction. We don't every want to create a wrapper for these functions, since they shouldn't be emitted. I'll add a better comment here.
That's not quite what this tests for. This would return true for a function that does work but ends in unreachable, say printing a fatal error message and calling abort. If you want to check for the 1-instruction function with unreachable, use isa<UnreachableInst>(&F.getEntryBlock().front()).


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:60
+  Function *rewriteVarargs(Function &F, Type *&VAListTy);
+  Function *createWrapper(Function &F, const std::vector<Use *> &AddressUses);
+  void createWrapperBody(Function *Wrapper, Function *Callee, Type *VAListTy);
----------------
Consider choosing SmallVectorImpl<Use *>?
http://llvm.org/docs/ProgrammersManual.html#sequential-containers-std-vector-std-list-etc


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:140
+  } else if (auto C = dyn_cast<Constant>(U->getUser())) {
+    if (Constants.insert(C).second)       // Constant::handleOperandChange must
+      C->handleOperandChange(F, Wrapper); // not be called more than once per user
----------------
Please move the comment to before the code instead of using two-column code // comments.


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:150
+  std::string Name = F.getName();
+  F.setName(Name + (F.isVarArg() ? OrigVASuffix : OrigSuffix));
+
----------------
Use llvm::Twine which is "a lightweight data structure for efficiently representing the concatenation of temporary values as strings." Value::setName takes a Twine argument. Something like:
  F.setName(Twine(F.getName(), F.isVarArg() ? OrigVASuffix : OrigSuffix));


https://reviews.llvm.org/D37581