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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.

A couple of things, but otherwise this LGTM. We can iterate and add this this in tree (my preference is to get his basic infrastructure committed so that the commits that add deduction for particular attributes can all be separate).



================
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
----------------
jdoerfert wrote:
> 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.
Okay. I don't like theoretically-unsound checks (that aren't explicitly labeled as such), however, and the fact that this is enabled by default when EXPENSIVE_CHECKS is defined isn't good if this isn't sound. Please update the comment in the cl::opt to indicate that this might spuriously fail for pessimistic fixed points, and don't enable this by default with EXPENSIVE_CHECKS.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:399
+    default:
+      break;
+    }
----------------
Please add a comment here explaining what gets added here (so that it's clear why it's here and empty).


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

https://reviews.llvm.org/D59918