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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks, that's a very good start! Few comments inline, I didn't understand everything.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:59
   HotnessType Hotness = HotnessType::Unknown;
+  MemoryAccessKind MemoryAccess = MemoryAccessKind::MAK_MayWrite;
 
----------------
I think we may get to the point with this structure where documenting fields here would be nice.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:293
 
+  struct FFlags {
+    unsigned ReadNone : 1;
----------------
Doc.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:365
 
+  bool updateCallGraphEdgeMAK(GlobalValue::GUID CalleeGUID,
+                              MemoryAccessKind M) {
----------------
Doc (including return value).

I'd suggest naming it: ```updateCalleeMemAccessKind()```


================
Comment at: include/llvm/Transforms/IPO/FunctionAttrs.h:36
+    Function &F, AAResults &AAR,
+    std::map<Function *, MemoryAccessKind> *CallSiteMAKs = nullptr);
 
----------------
DenseMap instead of std::map?


================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:106
 
+void thinLinkComputeFunctionAttrs(ModuleSummaryIndex &Index);
+
----------------
Doc (simple).


================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:129
 
+void thinLTOInsertFunctionAttrsForModule(Module &TheModule,
+                                         const ModuleSummaryIndex &Index);
----------------
Doc.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:551
 
+void llvm::thinLTOInsertFunctionAttrsForModule(
+    Module &TheModule, const ModuleSummaryIndex &Index) {
----------------
ncharlie wrote:
> Not sure if this is the best place for this function.
Agree, but coherent with the one right below...


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:424
 
+void llvm::thinLinkComputeFunctionAttrs(ModuleSummaryIndex &Index) {
+  auto checkCalls =
----------------
Here I'd expect a better description of "how" you're gonna achieve the API contract, i.e. a high level view about what's involved.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:425
+void llvm::thinLinkComputeFunctionAttrs(ModuleSummaryIndex &Index) {
+  auto checkCalls =
+      [](FunctionSummary *F,
----------------
naming could help understand. Something like ```all_of_calllees` or something like that (may still need a comment.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:427
+      [](FunctionSummary *F,
+         std::function<bool(FunctionSummary *, const CalleeInfo *)> Cond) {
+        for (auto &C : F->calls()) {
----------------
```std::function``` is heavy, use ```llvm::function_ref``` here.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:430
+          for (auto &V : C.first.getSummaryList()) {
+            FunctionSummary *S = cast<FunctionSummary>(V.get());
+            if (!Cond(S, &C.second))
----------------
Are we sure there can't be an alias summary here?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:440
+    for (auto &S : P.second.SummaryList)
+      if (FunctionSummary *F = dyn_cast<FunctionSummary>(S.get())) {
+        // TODO: propagate NoRecurseTopdown
----------------
early continue:

```
FunctionSummary *F = dyn_cast<FunctionSummary>(S.get());
if (!F) continue;
...
```


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:443
+        bool noRecursiveCallsInFunction =
+            checkCalls(F, [F](FunctionSummary *S, const CalleeInfo *C) {
+              if (!S->fflags().NoRecurse ||
----------------
Doc.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:445
+              if (!S->fflags().NoRecurse ||
+                  S->getOriginalName() == F->getOriginalName())
+                return false;
----------------
Not sure why the second condition is needed?


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+      FS->updateCallGraphEdgeMAK(P.first->getGUID(), P.second);
+    }
+  }
----------------
Why do we need to mark the edges?


https://reviews.llvm.org/D35633