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

[Johannes Doerfert via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:25
+// information to other abstract attributes in-flight but we might not want to
+// manifest the information. The Attributor allows to query in-flight abstract
+// attributes through the `Attributor::getAAFor` method. If used by an abstract
----------------
hfinkel wrote:
> An example would be useful here for explaining the abstract attributes "(e,g,, something)".
I have an example in the description of `getAAFor` below. I will add "(see theAttributor::getAAFor description)" here.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:48
+// find another justification or limit the optimistic assumes made.
+//
+// Change is the key in this framework. Until a state of no-change, thus a
----------------
hfinkel wrote:
> I'm not yet sure how to think about this. Many current attributes are derives with the help of LLVM utility functions which walk use-def chains, etc. How can an attributor use those utilities and know on what else it depends?
> 
> Or are we going to enhance all utilities to return an optional dependency vector or similar?
> 
> Or is the idea that we don't change the IR until the end, so all such utilities see only pessimistic information?
> Or is the idea that we don't change the IR until the end, so all such utilities see only pessimistic information?

I strongly urge us *not* to change the IR until the end. That is what I have done so far in all the following attributor patches. Trying to clean up after optimistically modifying the IR seems like a very fragile approach. It breaks with caches all over the place and with the common concept of the IR as the origin of truth.

> Or are we going to enhance all utilities to return an optional dependency vector or similar?

We can do that if necessary. I haven't so far. Just for the record, current way of using utilities doesn't benefit from "in-flight" information either. All new deductions have been strictly more powerful on the LLVM-TS and SPEC2006 benchmarks (if I remember correctly).


> I'm not yet sure how to think about this. Many current attributes are derives with the help of LLVM utility functions which walk use-def chains, etc. How can an attributor use those utilities and know on what else it depends?

I have various patches to derive attributes already on phabricator so we can actually look at them in detail (see the Stack button above). Some design decisions are described in the class comment of the Attributor but let me rephrase with regards to your question:

If you use utility functions you get potentially pessimistic answers. You can walk def-use chains, etc., just fine but you should query attributes in flight whenever possible. By construction, these attributes should provide the best possible result. An attribute can rely on utility functions as well but should only do so after other means failed to provide justification for a better state. I don't exclude that we might want to re-implement/enhance utility functions at some point such that they can, in turn, use in-flight attributes.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:365
+  // re-initialization of the arguments. This re-run should not result in an IR
+  // change. Though, the (virtual) state of attributes at the end of the re-run
+  // might be more optimistic than the known state or the IR state if the better
----------------
hfinkel wrote:
> This is true even if we did't reach the fixpoint and has to reset some attributes to their pessimistic state?
[I read it as "Is this true...".]

No it is not. If we do not reach an optimistic fixpoint we force a pessimistic one. If we modify the IR, we can, in the re-run, again find a better stable state (which could be an optimistic or pessimistic fixpoint) that allows again to enhance the IR. 

After thinking about the use of utility functions a bit more, I guess this can actually also happen if we have a fixpoint based on pessimistic information. Maybe we have to weaken the "llvm_unreachable" at some point, or we have to avoid/extend utility functions. For now, and with up to 32 iterations, I haven't seen a single non-optimistic fixpoint in all the test runs on LLVM-TS and SPEC2006.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:401
+      break;
+    }
+    if (IsInterestingOpcode)
----------------
hfinkel wrote:
> Are there supposed to be some other cases here?
> Are there supposed to be some other cases here?

Yes, at some point. In this patch *no* attributes are registered or derived and we also do not have "interesting" instructions yet. They are introduced with the attributes patches (see dependent patches).

I didn't know a better way to split this. I opted to keep the framework together and put the attributes in separate patches. If you prefer it any other way, I'd be more than happy do change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918