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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D62766#1531219 <https://reviews.llvm.org/D62766#1531219>, @sstefan1 wrote:

> Inline comments are now not in original order, so I'll reply here.
>
> > I think the`!` in front of ICS is a problem. Did you run this?
>
> Yes.


So remove it ;)

>> Why is it sufficient that one ordering is "weak enough"? Don't we have to test both? Either way, we need a comment to explain what is happening here.
> 
> My thinking is that if either one of them 'weak enough', than "no-sync" is no longer possible since at any point it can be one of the orderings. If you disagree, I can change and require both.

I mixed up the meaning of the return value. It looks fine once I read the comment.

>> Indention. And maybe add a few more words here ;)
> 
> I updated the function comment, hope thats enough.

Looks good.

I added more comments but I think this is almost done. Go through the code and tests yourself and make sure there is no spurious newlines or other changes you did not intend.



================
Comment at: llvm/docs/LangRef.rst:1477
+    This function attribute indicates that the function does not communicate
+    (synchronize) with another thread.
 ``nounwind``
----------------
Maybe add something like:
> If the function does ever synchronize with another thread, the behavior is undefined.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:647
 
+struct AANoSync : public AbstractAttribute, BooleanState {
+  /// An abstract interface for all nosync attributes.
----------------
You don't need to inherit from `BooleanState` here. That is an implementation detail we probably want to hide. Let `AANoSyncFunction` inherit from `BooleanState` but keep the functions `isAssumedNoSync` and `isKnownNoSync` here. They will not have an implementation and are overwritten in `AANoSyncFunction`.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:667
+  bool isKnownNoSync() const { return getKnown(); }
+}; // namespace llvm
+
----------------
Copy and paste, this is not a namespace ;)


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:115
+define void @volatile_store(i32*) norecurse nounwind uwtable {
+    ;store volatile i32 14, i32* %0, align 4, !tbaa !2
+      store volatile i32 14, i32* %0, align 4
----------------
Remove the commented instruction here and in the next test. Also, fix the indention.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:139
+; TEST 8 
+declare void @nosync_function() noinline nounwind uwtable
+
----------------
Isn't the "nosync" attribute missing for this function?


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:197
+;
+; void foo(){
+;   while(!flag.load(std::memory_order_relaxed))
----------------
The function names don't match the IR names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766