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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I added last minor comments from my side. Other than that I think this looks fine. We will have to wait for the others though.

(You will need to rebase and make sure `ninja check-all` passes because there are other new attributes.)



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:357
+        return false;
+      return true;
+    }
----------------
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)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:412
+    for (Instruction *I : OpcodeInstMap[Opcode]) {
+      if (I->mayReadOrWriteMemory())
+        continue;
----------------
I was puzzled by this check for a second, add a comment indicating that the above loop handles calls with read/write effects already. Mention that the fact there is a read/write effect caused us already to make sure it is `nosync` and there is consequently no need to check for `convergent`.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:311
+
+; TEST 16 - positive, non-volatile intrinsic.
+
----------------
Copy&paste


================
Comment at: llvm/test/Transforms/FunctionAttrs/nosync.ll:350
+; ATTRIBUTOR: Function Attrs: nounwind
+; ATTRIBUTOR-NOT: nosync
+; ATTRIBUTOR-NEXT: define i32 @cos_test(float %x)
----------------
Shouldn't this be `nosync`? Is it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62766