[PATCH] D99769: [funcattrs] Infer nosync from instruction walk

Thu Apr 8 03:58:19 PDT 2021

fhahn added a comment.

In D99769#2665277 <https://reviews.llvm.org/D99769#2665277>, @reames wrote:

> In D99769#2665218 <https://reviews.llvm.org/D99769#2665218>, @jdoerfert wrote:
>
>>> Pretty straightforward use of existing infrastructure and port of the attributor inference rules for nosync.
>>
>> port = duplication, where does this stop?
>
> When the folks working on attributor do the work to stabilize it and get it on by default.  I don't know exactly how much work that is, but I suspect quite a bit.

The duplication is a bit unfortunate, but it provides a stable path forward, with less uncertainty than the lightweight attributor pass. At the moment, FunctionAttrs is the default way to do this kind of reasoning and IMO this 'duplication' seems like a reasonable trade-off, especially if Philip is happy to provide a patch even if the code becomes unused at some point. I think this also provides indirect benefits for the attributor, as it exposes a small subset of the reasoning by default and enables other passes to make use of the additional attributes right now.

In D99769#2676226 <https://reviews.llvm.org/D99769#2676226>, @sstefan1 wrote:

> I'm doing some testing with a lightweight attributor enabled by default. Can this wait few more days, until I get some results? If the numbers are good, maybe we won't need this after all?

That's great, but I think 'using a lightweight attributor pass by default' requires at least the following steps 1) add the pass with option to enable it, 2) have people test it, 3) collect data to make a convincing case for enabling it, 4) flip the default and deal with the fallout. I am not sure this can be done in a few days.

I'd recommend to start with sharing a patch that adds this lightweight attributor pass, so we can start with getting consensus what the scope of this pass should be and go from there (without tying ourselves to any particular timeframe)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99769