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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 2 inline comments as done.
sstefan1 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:357
+        return false;
+      return true;
+    }
----------------
jdoerfert wrote:
> I think the "unknown" case is handled the wrong way here. Shouldn't it be:
> ```
>       if (Arg->getType()->isIntegerTy(1) &&
>           cast<ConstantInt>(Arg)->getValue() == 0)
>         return true;
>       return false;
> ```
> such that "unknown" values, e.g., `%cmp = icmp ...` used as the 4th argument will conservatively make it sync?
> 
> (+ Test case for this)
4th argument, isvolatile, is `immarg`, so I guess this not necessary?


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:350
+; ATTRIBUTOR: Function Attrs: nounwind
+; ATTRIBUTOR-NOT: nosync
+; ATTRIBUTOR-NEXT: define i32 @cos_test(float %x)
----------------
jdoerfert wrote:
> Shouldn't this be `nosync`? Is it?
Yes, this falls under copy & paste as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766