[PATCH] D59918: [Attributor] Pass infrastructure and fixpoint framework

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:240
+
+  /// Determine opportunities to derive 'default' attributes in \p F and create
+  /// abstract attribute objects for them.
----------------
I am not sure "Determine opportunities to derive 'default' attributes ' is very clear. This is where we add the initial abstract attributes to the state and also collect the IR info, right? Maybe worth stating that a bit more explicitly.


I am not sure what the long term plan for the distinction between 'default' and other attributes is, but it might be worth to only add this notation once it is required?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:275
+  using QueryMapTy =
+      DenseMap<AbstractAttribute *, SmallPtrSet<AbstractAttribute *, 32>>;
+  QueryMapTy QueryMap;
----------------
32 entries per attribute seems a bit large at a first look. Might be interesting to gather some statistics on the average number of dependencies.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:273
+      auto &QuerriedAAs = QueryMap[ChangedAA];
+      Worklist.insert(QuerriedAAs.begin(), QuerriedAAs.end());
+    }
----------------
QueriedAAs is a SmallPtrSet, so the order we insert items into the worklist here is not deterministic. Is there anything preventing us from reaching different fixed points when iterating over items in different orders?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:434
+
+  // Create an attributor and initially empty informatin cache that is filled
+  // while we identify default attribute opportunities.
----------------
nit: information.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:449
+    if (!F->hasExactDefinition()) {
+
+      // Bookkeeping.
----------------
spurious newline?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:460
+
+    // Bookkeeping.
+    NumFnWithExactDefinition++;
----------------
IMO this and above comment does not add any meaningful information.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:463
+
+    // Sanity check.
+    assert(!F->isDeclaration() && "Expected a definition not a declaration!");
----------------
IMO this can also be dropped, as the string in the assert gives enough details about the check.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:467
+    // Populate the attributor with abstract attribute opportunities in the
+    // function and te information cache with IR information.
+    A.identifyDefaultAbstractAttributes(*F, InfoCache);
----------------
te -> the


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:485
+
+  if (runAttributorOnSCC(SCCFunctions))
+    return PreservedAnalyses::all();
----------------
runAttributorOnSCC(SCCFunctions) returns true if it changed the IR, right? Shouldn't the return values be switched, i.e. do not preserve anything, if something changed?

As a side note, shouldn't we be able to preserve any analysis that does not make use of attributes? From a correctness perspective, we should be able to preserve all analysis, but we would like to invalidate those that could make use of the additional info. AFAIK at least in the new pass manager, it should be possible to preserve function-level analysis in a module/SCC pass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59918/new/

https://reviews.llvm.org/D59918