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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D99769#2676275 <https://reviews.llvm.org/D99769#2676275>, @fhahn wrote:

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

What I'm saying is that it seems off to argue for duplication of code/work to avoid work, especially since this makes the end goal harder to achieve.
TBH, I believe the judgement would look different if someone would propose workaround #5 that duplicates functionality to avoid looking into a proper solution.

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

This is arguably only true if people that fix errors in the FunctionAttr pass transfer those fixes and tests to the Attributor.
As with all duplication this is likely not to happen at all, or maybe just not all the time. I certainly doubt it will.
I see the point for a solution now, obviously, however, we could at least try to minimize duplication so fixes would transfer automatically.
For example, why do we need the (basically) same static methods twice instead of exposing them?
`static AANoSync::isNoSyncIntrinsic` could be called by the FunctionAttr pass and we would actually expose a subset of the reasoning through the same code.
Asking to expose common functionality in helper functions is really not something specific to the Attributor and there needs to be a good reason to avoid it in favor of copy & paste.

---

All in all, I don't think the logic that is applied here wrt. development is sensible. If nobody even tries to run the Attributor to solve their problem but
instead cherry picks logic out of it, and copies it, we are moving away from enabling it and not closer. To be concrete, what I am missing here is a sentence that reads like:
"I tried to alternatively run the Attributor only for `nosync` deduction. I created and Attributor object in the `addNoSyncAttr` method, restricted deduction
 to `AANoSync` and seeded the functions I was interested in. The result was ... and therefore we should ...."
You can argue this is too much work but I doubt it is, and it seems people are guessing mostly when it comes to this which leads me to believe they haven't tried.
I also doubt we would make the same decisions in other places. I mean, it is common to ask people to compare to alternative designs, especially if those are already
available. FWIW, setting up a single attribute deduction with the Attributor should be a much easier task than requests to implement an alternative solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99769