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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1199
+  case Attribute::NoSync:
+    return 1ULL << 62;
   case Attribute::Dereferenceable:
----------------
I think, you can remove this change. All should be fine without.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:357
+        return false;
+      return true;
+    }
----------------
sstefan1 wrote:
> 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?
Agreed, not necessary. However, if you keep it this way, add the above reasoning to the comment, it confused me now and it can easily confuse the next person. My advice, just swap the order to make it easier for people now and in the future ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766