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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D59918#1469666 <https://reviews.llvm.org/D59918#1469666>, @jdoerfert wrote:

> In D59918#1467443 <https://reviews.llvm.org/D59918#1467443>, @fhahn wrote:
>
> > And finally, what can we do to check correctness? As the current attribute definitions are a bit fuzzy, it seems this could lead to problems down the road, in case we infer invalid attributes, which only get used (and cause problems) a bit later. Assuming we have a clear definition of a set of attributes, how hard would it be to verify the attributes in a module (maybe we are doing it already)?
>
>
> I'm unsure I understand this question. If a fixpoint is reached, as reported by the `AbstractAttribute::updateImpl` methods, we should have derived "correct" information. Obviously, there might be a bug in the logic of the `AbstractAttribute::updateImpl` function, or the associated materialization, the dependence tracking in the attributor, etc. but I do not have a better solution than more (stress) tests (see D59903 <https://reviews.llvm.org/D59903>). I open a few bugs while I was developing attribute deductions in this framework and compare the results to the existing one. We current derive different things we should not as described in PR41336 and PR41328. More attributes will also trigger, or better expose, "bugs" down the line (see D59917 <https://reviews.llvm.org/D59917>) and we will probably have to manually look into the root cause as they pop up, was the attribute not correct or later reasoning based on it.


Right, my thinking for checking correctness was along the following lines: Given some input IR with attributes, it should be fairly easy to check if (some of the) provided attributes are consistent with each other and the IR, right? At least easier than deducing them ;) I was wondering if we could implement a function/module verifier to check certain attributes. That way, we could check that certain types of attributes deduced are consistent with the underlying IR, without the attributes being used for some other optimization.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:29
+// automatically capture a potential dependence from Q to P. This dependence
+// will automagically cause P to be reevaluated whenever Q changes.
+//
----------------
nit: not sure if automagically is the best way to put it.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:271
+
+/// Base class for all "concrete attribute" deductions.
+///
----------------
it's defined as struct?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:78
+/// Helper to adjust the statistics.
+static void bookkeeping(AbstractAttribute::ManifestPosition MP,
+                        const Attribute &Attr) {
----------------
This doesn't do anything at the moment?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:106
+/// Return true if \p New is equal or worse than \p Old.
+static bool isEqualOrWorse(const Attribute &New, const Attribute &Old) {
+  if (!Old.isIntAttribute())
----------------
Would it make sense to put this in Attribute?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:110
+
+  if (Old.getValueAsInt() >= New.getValueAsInt())
+    return true;
----------------
could be shortened to just `return Old.getValueAsInt() >= New.getValueAsInt()`



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:319
+    for (AbstractAttribute *AA : Worklist)
+      if (AA->update(*this) == ChangeStatus::CHANGED)
+        ChangedAAs.push_back(AA);
----------------
Passing in the whole Attributor seems a bit heavy handed. 

Conceptually, IIUC, to update an attribute, we just need a way to query other attributes (and potentially some other IR info) as input from the attributor, right?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:451
+
+  for (Instruction &I : instructions(&F)) {
+    bool IsInterestingOpcode = false;
----------------
Is the plan here for the Attributor to provide an interface for the Attribute implementations to query "interesting" instructions, instead of the Attribute implementations looking at the IR themselves? It seems this might impose a tighter coupling of Attributes <-> Attributor than intended with the flexible attribute registration system.

Related to the comment at line 319, it might be worth splitting this into a more restricted query/info interface that Attribute::update can use. It seems not related to the fixed point iteration in Attributor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918