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

Tue May 7 15:53:07 PDT 2019

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

In D59918#1493987 <https://reviews.llvm.org/D59918#1493987>, @fhahn wrote:

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

Yes, we could implement this check, and yes doing so in this framework would probably help. Unfortunately, the logic we need for determining attributes and the one we need to "verify" attributes is only similar but often not the same. That said, we can most probably share a lot of it. So if that is something we want to look into I would propose to add `AbstractAttribute::verify` functionality which, if overloaded, will try to "disprove" existing attributes and report them as errors/warnings/notes/remarks/... Some of the logic in the `updateImpl` and `verify` is then overlapping for most attributes. It is unclear to me right now if you would have any need for the fixpoint framework though. I guess you'll end up with a separate "AttributeVerify" pass instead.

================
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.
+//
----------------
fhahn wrote:
> nit: not sure if automagically is the best way to put it.
I'll remove the //magic// :(

================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:271
+
+/// Base class for all "concrete attribute" deductions.
+///
----------------
fhahn wrote:
> it's defined as struct?
True, but there is little difference anyway, right? I use structs here as there are no members.
You want me to use "struct" in the comment as well?

================
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())
----------------
fhahn wrote:
> Would it make sense to put this in Attribute?
Potentially, yes. I don't right now where we would need similar functionality but I can most certainly move it there either way.

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

================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:319
+    for (AbstractAttribute *AA : Worklist)
+      if (AA->update(*this) == ChangeStatus::CHANGED)
+        ChangedAAs.push_back(AA);
----------------
fhahn wrote:
> 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?
Correct, query other in-flight attributes and cached IR information.

I pass the Attributor because the above functionality is pretty much everything it exposes as public interface anyway (I think). Would you like to have another thin interface layer on top?

================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:451
+
+  for (Instruction &I : instructions(&F)) {
+    bool IsInterestingOpcode = false;
----------------
fhahn wrote:
> 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.
The first part goes into a very interesting direction I have thought about as well. Without doing performance tests (mostly because I didn't have any attributes etc.) I choose this solution for "performance" reasons. If we invert it, which we still can and which I do not oppose, every attribute is queried for every instruction in order to decide if it is interesting to cache it, at least in the naive implementation I have in mind right now.

If we can come up with a good interface to expose IR information, probably cached, that would be great. If we put that, and the attribute query interface outside the Attributor, that would be OK. Though, the attributor will need to know about attribute queries (now through `Attributor::getAAFor<...>(...)` to track dependencies).

So far I decided this coupling is not too bad even though we have to hardcode "interesting" here. If no one has super strong feelings about this, I'd argue we go with this for now and once we have a good handle on the use cases we try to design a clean interface. But again, I will not oppose any proposal for such an interface right now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918