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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

Please add tests for the things I mention in comments, as well as:

- relaxed volatile atomic load / store
- inline assembly



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:381
+struct IntegerState : public AbstractState {
+  /// Undrlying integer type, we assume 32 bits to be enough.
+  using base_t = uint32_t;
----------------
"Underlying"


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:314
+  case Instruction::Fence:
+    Ordering = cast<FenceInst>(I)->getOrdering();
+    break;
----------------
A fence with `singlethread` sync scope doesn't sync with other threads, even if `seq_cst`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:331
+    // Unknown atomic, assume non-relaxed.
+    return true;
+  }
----------------
We probably want `llvm_unreachable` here, so the code gets updated if we add new atomic operations.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:341
+
+bool AANoSyncFunction::isVolatile(Instruction *I) {
+  switch (I->getOpcode()) {
----------------
You're missing `memcpy` and similar intrinsics, I think you want to handle them here and not in the generic intrinsic handling.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:5
+
+; Test cases designet for the nosync function attribute.
+; FIXME's are used to indicate problems and missing attributes.
----------------
"designet"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766