[PATCH] D68491: [PATCH 08/38] [noalias] [IR] IRBuilder support for noalias intrinsics.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:651
+        AllocaPtr,
+        ConstantInt::get(IntegerType::getInt32Ty(getContext()), ObjId), Scope);
+  }
----------------
But `ObjId` is `uint64_t`?


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:654
+  Instruction *CreateNoAliasDeclaration(Value *AllocaPtr, MDNode *ScopeTag) {
+    uint64_t zero = 0;
+    return CreateNoAliasDeclaration(AllocaPtr, zero,
----------------
Here and elsewhere - currently llvm variable naming scheme did not change, all variables start with Uppercase Letter.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:663
+                                    const Twine &Name = "",
+                                    unsigned ObjectId = 0) {
+    return CreateNoAliasPointer(Ptr, NoAliasDecl, AddrP,
----------------
Should this be `uint32_t` ?


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:671
+                                    const Twine &Name = "",
+                                    unsigned ObjectId = 0);
+
----------------
Should this be `uint32_t` ?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:889-890
+                                       const IntrinsicInst *rhs) {
+    assert(lhs->getIntrinsicID() == Intrinsic::side_noalias);
+    assert(rhs->getIntrinsicID() == Intrinsic::side_noalias);
+    return (lhs->getOperand(Intrinsic::SideNoAliasScopeArg) ==
----------------
assert(lhs->getIntrinsicID() == rhs->getIntrinsicID() && ;hs->getIntrinsicID() == Intrinsic::side_noalias && "Can only check noalias compatibility of side_noalias intrinsics");



================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:891-896
+    return (lhs->getOperand(Intrinsic::SideNoAliasScopeArg) ==
+            rhs->getOperand(Intrinsic::SideNoAliasScopeArg)) &&
+           (lhs->getOperand(Intrinsic::SideNoAliasIdentifyPObjIdArg) ==
+            rhs->getOperand(Intrinsic::SideNoAliasIdentifyPObjIdArg)) &&
+           (lhs->getOperand(Intrinsic::SideNoAliasIdentifyPArg) ==
+            rhs->getOperand(Intrinsic::SideNoAliasIdentifyPArg));
----------------
`std::tie(<...>) == std::tie(<...>)` ?


================
Comment at: llvm/lib/IR/IRBuilder.cpp:514
+IRBuilderBase::CreateNoAliasCopyGuard(Value *BasePtr, Value *NoAliasDecl,
+                                      ArrayRef<long long> EncodedIndices,
+                                      MDNode *ScopeTag, const Twine &Name) {
----------------
Should this be `uint64_t`?


================
Comment at: llvm/lib/IR/IRBuilder.cpp:565-566
+  // For the metadata info, types must not be added:
+  for (auto *MD : MDNodes) {
+    Ops.push_back(MetadataAsValue::get(Context, MD));
+  }
----------------
Ops.insert(Ops.end(), MDNodes.begin(), MDNodes.end()) ?


================
Comment at: llvm/lib/IR/IRBuilder.cpp:569
+  for (auto *MDV : MDValues) {
+    Ops.push_back(MDV);
+  }
----------------
same


================
Comment at: llvm/lib/IR/IRBuilder.cpp:573-577
+  auto *FnIntrinsic = Intrinsic::getDeclaration(M, ID, Types);
+  Instruction *Ret = createCallHelper(FnIntrinsic, Ops, this, Name);
 
   if (Ret->getType() != Ptr->getType()) {
+    BitCastInst *BCI = new BitCastInst(Ret, Ptr->getType(), Name + ".cast");
----------------
This should be in the parent patch that added that


================
Comment at: llvm/lib/IR/Verifier.cpp:4760-4761
+              NoAliasDeclarations.push_back(cast<CallBase>(V));
+            } else if (isa<PHINode>(V)) {
+              PHINode *PHI = cast<PHINode>(V);
+              for (auto *PHIOp : PHI->operand_values()) {
----------------
```
} else if (auto *PHI = dyn_cast<PHINode>(V)) {
```


================
Comment at: llvm/lib/IR/Verifier.cpp:4762-4763
+              PHINode *PHI = cast<PHINode>(V);
+              for (auto *PHIOp : PHI->operand_values()) {
+                Worklist.push_back(PHIOp);
+              }
----------------
Worklist.insert() ?


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

https://reviews.llvm.org/D68491