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

Tue Apr 16 18:29:20 PDT 2019

jdoerfert added a comment.

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

> I haven't been through the details yet, but I have a few high-level thoughts.

Thanks for starting the review process!

> This is a lot of code and I think a high-level description of the framework would be very helpful,

Agreed. I will add a high-level description (as a comment) tomorrow to sum up how the framework is supposed to work/be used. Some details are included below.

> like what are the abstract states/lattice values,

This is up to the individual abstract attributes (which are introduced in the other patches). Generally, the `Attributor` assumes the `AbstractAttribute::updateImpl` method will be monotone and the underlying lattice is of finite depth. That being said, the iteration threshold and the fact that abstract attributes have to allow to fall back to a known correct solution, should make this framework behave correctly (and terminate) even if the properties are not fulfilled.

> how do we transition between them, what is the strategy for iterating over functions, how are we applying & materializing updates, how expensive is this analysis?

The `Attributor` calls the `AbstractAttribute::updateImpl` method on all abstract attributes that might have been impacted by changes in the last iteration of the analysis. There is currently no particular order specified but it is important to note that we look at all "potentially impacted abstract attributes" not the underlying IR. That means we often do not need to rerun analysis on the whole module every iteration.

The set of "potentially impacted abstract attributes" is collected on the fly through a hook in the `Attributor::getAAFor` method. If an abstract attribute P queries the abstract attribute Q (for whatever reason! Note that Q could be P itself), a dependence from P to Q is recorded. If Q ever changes in the future, the `updateImpl` method of P is invoked in the next iteration.

If an optimistic fixpoint is reached, because no changes happened in an iteration, or a pessimistic fixpoint is enforced, due to the iteration threshold, we manifest all abstract attributes that are improving the status quo.

Materialization is generally up to the abstract attributes. There are default methods in the `AbstractAttribute` class that, with minimal overloading effort, cover general cases, e.g., one attribute on a function, argument, return value, etc.. Slight variations from this norm can be achieved by overloading the helper functions, e.g., `AbstractAttribute:getDeducedAttributes`, for more elaborate schemes the `AbstractAttribute::manifest` method can be overloaded.

The cost is "empirically non-existent" on LLVM-TS and SPEC2006 when I run with all the abstract attributes that are up for review. This patch adds virtually no overhead on its own.
For my experiments, I collected samples of (I think) 11 runs of a fully loaded system with 112 threads. There was always (= for each of the patches in this patch set) at most one significant compile and/or runtime change reported by lnt, the difference was very small and it sometimes improved and sometimes regressed.

> As this seems to be a generic framework it would be helpful to describe how to hook up a new attribute for inference.

Again agreed. As part of the overview which I forgot to include and mentioned above, I'll describe the necessary steps. For now, if you are interested, you can take a look at D60074 <https://reviews.llvm.org/D60074>, probably the smallest patch I have that adds a new attribute.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918