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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.

I'll update the patch with the requested (small) changes asap.

Are there more design comments? (@chandlerc?)



================
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.
----------------
fhahn wrote:
> 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?
Mh, fair point. Though, I think it makes sense to have a set of attributes that live here and the ability to add some that do not.

Would it help if I list the "default" attributes? Do you think I should hide this interface for now?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:275
+  using QueryMapTy =
+      DenseMap<AbstractAttribute *, SmallPtrSet<AbstractAttribute *, 32>>;
+  QueryMapTy QueryMap;
----------------
fhahn wrote:
> 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.
Fair point. I can do that with the attributes we have. (or ask the GSoC students to do it).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:273
+      auto &QuerriedAAs = QueryMap[ChangedAA];
+      Worklist.insert(QuerriedAAs.begin(), QuerriedAAs.end());
+    }
----------------
fhahn wrote:
> 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?
Good catch. Short answer, if the attribute update methods are sane and an optimistic fixpoint is reached, we should get a unique one. If not, there might be different ones reached.

I can make it all deterministic.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:485
+
+  if (runAttributorOnSCC(SCCFunctions))
+    return PreservedAnalyses::all();
----------------
fhahn wrote:
> 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.
Good catch.

> 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.

I should be, and we should list some. I'll look into it. I *think*, right now the placement ensures that we do not invalidate anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918