Page MenuHomePhabricator

[funcattrs] Infer nosync from instruction walk
ClosedPublic

Authored by reames on Thu, Apr 1, 2:12 PM.

Details

Summary

Pretty straightforward use of existing infrastructure and port of the attributor inference rules for nosync.

A couple points of interest:

  • I deliberately switched from "monotonic or better" to "unordered or better". This is simply me being conservative and is better in line with the rest of the optimizer. We treat monotonic conservatively pretty much everywhere.
  • The operand bundle test change is suspicious. It looks like we might have missed something here, but if so, it's an issue with the existing nofree inference as well. I'm going to take a closer look at that separately.
  • I needed to keep the previous inference from readnone. This surprised me, but made sense once I realized readonly inference goes to lengths to reason about local vs non-local memory and that writes to local memory are okay. This is fine for the purpose of nosync, but would e.g. prevent us from inferring nofree from readnone - which is slightly surprising.

Diff Detail

Event Timeline

reames created this revision.Thu, Apr 1, 2:12 PM
reames requested review of this revision.Thu, Apr 1, 2:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: bbn, wdng. · View Herald Transcript

Pretty straightforward use of existing infrastructure and port of the attributor inference rules for nosync.

port = duplication, where does this stop?

reames added a comment.Thu, Apr 1, 3:00 PM

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.

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?

fhahn added a subscriber: fhahn.Thu, Apr 8, 3:58 AM

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.

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)

reames added a comment.Thu, Apr 8, 9:52 AM

+1 to @fhahn's comments

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)

Sounds good to me. I agree it is better not to rush things.

Just for clarification: a lightweight attributor wouldn't be a separate pass, but a regular attributor pass just with fewer abstract attributes initialised by default. I'll start with that and we'll see how it goes.

sstefan1 accepted this revision.Thu, Apr 8, 1:26 PM
This revision is now accepted and ready to land.Thu, Apr 8, 1:26 PM
This revision was landed with ongoing or failed builds.Thu, Apr 8, 2:05 PM
This revision was automatically updated to reflect the committed changes.

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.

reames reopened this revision.Tue, Apr 13, 9:48 AM

Johannes,

Sorry for not replying to your points inline, phab makes that difficult for long intermixed replies.

I am glad to see that work is being done towards enabling Attributor by default. So as to avoid duplicating a long comment, I will point you to my comment on that review (https://reviews.llvm.org/D100339#2686153) to explain my concerns on that direction.

On the broader topic, I am happy to discuss the workflow implications offline. Honestly, I think you've got the wrong end of the stick here. If I accept the premise of your argument, my actual takeaway is that I should resist all efforts to add experimental code before it has been widely discussed and accepted as the agreed upon path forward. I think we can all agree that's not a desirable outcome. I also think it's out of sync with long standing practice in the community.

Again, if you want to talk on the workflow stuff, let's talk offline. There's a bunch of subtleties that is far too easy to loose in written conversation. I, in particular, seem very prone to that. :)

This revision is now accepted and ready to land.Tue, Apr 13, 9:48 AM
reames closed this revision.Tue, Apr 13, 2:56 PM

Had not intended to reopen.