[PATCH] D100339: [Attributor] Run lightweight version of the Attributor by default.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

High level meta comment on enabling attributor by default, not on the code per se.

I think this deserves to be raised and discussed on llvm-dev.  You are essentially proposing that we adopt Attributor as our long term plan for IPO in LLVM.  This has not been widely discussed and agreed upon.  You need to send an RFC before enabling this by default.

I'm about to list a couple of concerns I have.  Please don't debate me here.  Instead, consider this advanced warning of the issues likely to come up in an llvm-dev thread, and have your data and arguments ready.  (i.e. These are probably concerns you want to address in your RFC.)

One major concern I have is the implications for compile time.  My understanding (which may be wrong) is that Attributor iterates to a fixed point between multiple attributes over the entire call graph.  The existing inference code intentionally and specifically does not do the same.  Fixed point iteration is potentially much more expensive then the existing post order walk.  I think this needs to be measured, and discussed explicitly on llvm-dev.  I could see an argument that we may be willing to accept the increased compile time at O3 <https://reviews.llvm.org/owners/package/3/>, but I am skeptical that this will be worthwhile at O2 <https://reviews.llvm.org/owners/package/2/> and below.  You need to make that argument, on llvm-dev, with evidence.

A second major concern I have is correctness.  To my knowledge, Attributor has not been extensively fuzzed or otherwise programmatically tested at scale.  Before this enabled (not before a flag is added), you need to do the work and/or make the argument that you have done the work, to justify a reasonable belief that Attributor is in fact as correct as the code it is replacing/supplementing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100339