[PATCH] D35633: [ThinLTO] Add FunctionAttr NoRecurse and ReadAttr propagation to ThinLTO

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This is a decent start, some comments inline. Thanks!



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:310
 
+  FFlags FunFlags;
+
----------------
Comment?


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:374-376
+    auto E = std::find_if(CallGraphEdgeList.begin(), CallGraphEdgeList.end(),
+                          [&CalleeGUID](FunctionSummary::EdgeTy &E) -> bool {
+                            return E.first.getGUID() == CalleeGUID;
----------------
Do you need the `-> bool` ?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:141-146
   for (inst_iterator II = inst_begin(F), E = inst_end(F); II != E; ++II) {
     Instruction *I = &*II;
 
     // Some instructions can be ignored even if they read or write memory.
     // Detect these now, skipping to the next instruction if one is found.
     CallSite CS(cast<Value>(I));
----------------
range for?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:154-156
+      case MAK_ReadOnly:
+        ReadsMemory = true;
+        LLVM_FALLTHROUGH;
----------------
I find this a little harder to read, maybe you can use an explicit `continue` instead of `LLVM_FALLTHROUGH` ?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1009
       F->addAttribute(AttributeList::ReturnIndex, Attribute::NonNull);
+
       ++NumNonNullReturn;
----------------
spurious new whiteline.


https://reviews.llvm.org/D35633