[PATCH] D62766: [Attributor] Deduce "nosync" function attribute.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle requested changes to this revision.
nhaehnle added a comment.
This revision now requires changes to proceed.

This does seem useful, although the description is overly narrow (what does nosync on its own have to do with freeing memory?).

I also think that the definition of `nosync` needs some work, as just "synchronization" is a rather vague term. Can you define it in terms of fences and atomic instructions instead, e.g. by saying that a nosync function does not perform such operations (or some subset of such operations)?



================
Comment at: llvm/docs/LangRef.rst:1476-1478
+    This function attribute indicates that the function does not communicate
+    (synchronize) with another thread. If the function does ever synchronize 
+    with another thread, the behavior is undefined.
----------------
arsenm wrote:
> jdoerfert wrote:
> > jdoerfert wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > I think this is a bit vague. In particular I don't think the LangRef defines what a "thread" means anywhere. I also think this needs to be more clear on what kinds of synchronization is allowed. Is this only communication through some addressable memory? What about GPU cross lane communication operations?
> > > > > 
> > > > > I'm wondering if this is sufficient to solve this problem: http://lists.llvm.org/pipermail/llvm-dev/2013-November/067359.html
> > > > > TLDR, memory instructions can currently be hoisted over an arbitrary call if they are accessing a noalias argument
> > > > This is also mentioned as a proper attribute here (which I would greatly prefer to adding another string attribute), but only handled as a string attribute
> > > That is a good point. I was initially thinking string attributes are fine but D62784 seems to be stuck which makes the testing of them hard.
> > > 
> > > Long story short, lets make them enum attributes.
> > > 
> > > @sstefan1 could you please make this a proper enum attribute? This will require some additional "mechanics" in:
> > > `llvm/lib/AsmParser/LLParser.cpp`
> > > `llvm/lib/Bitcode/Reader/BitcodeReader.cpp`
> > > `llvm/lib/Bitcode/Writer/BitcodeWriter.cpp`
> > > `llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp`
> > > `llvm/lib/IR/Attributes.cpp`
> > > `llvm/lib/IR/Verifier.cpp`
> > > 
> > > Could be more though. Look for an existing attribute, e.g. Cold, and how that is handled.
> > > 
> > > 
> > > @uenoku Could you please also make `nofree` an enum attribute?
> > > I think this is a bit vague. In particular I don't think the LangRef defines what a "thread" means anywhere.
> > 
> > I did think/hope we do not have to. There is the implicit execution thread and `nosync` says there is "nothing else" while the function is executed. Basically, there are no side-effects that did not originate from the code we see. Please object if you think this is not sufficient.
> > 
> > > I also think this needs to be more clear on what kinds of synchronization is allowed. 
> > 
> > None, if `nosync` is present.
> > 
> > > Is this only communication through some addressable memory? What about GPU cross lane communication operations? 
> > 
> > I'd say, not allowed if `nosync` is present.
> > 
> > > TLDR, memory instructions can currently be hoisted over an arbitrary call if they are accessing a noalias argument
> > 
> > I tried to expose that lately [1] but failed, do you have an example?
> > 
> > [1] https://bugs.llvm.org/show_bug.cgi?id=41781
> Is it then disallowed to merge any calls that aren't nosync? e.g.
> 
> ```
> if (foo)
>   bar(x) readnone
> else
>   bar(y) readnone
>   
> ```
> 
> is no longer legal to combine these as
> 
> ```
>   
> bar(foo ? x : y) readnone
>   
> ```
No, I think that would still be allowed. The sync (aka not-nosync) functions have a potential side effect in terms of the memory model, but it's the same side effect in either case since the memory model at this point doesn't care about subgroups.

I guess you're thinking of subgroup operations, but the issue with those is that the set of threads with which communication occurs is a function of where the operation occurs in control flow. It makes sense to keep that issue separate from this attribute.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:293-295
+; TEST 14 - negative, checking volatile intrinsics.
+
+; ATTRIBUTOR: define i32 @memcpy_volatile(i8* %ptr1, i8* %ptr2)
----------------
The negative check line here is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766