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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere marked 5 inline comments as done.
jeroen.dobbelaere added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:651
+        AllocaPtr,
+        ConstantInt::get(IntegerType::getInt32Ty(getContext()), ObjId), Scope);
+  }
----------------
lebedev.ri wrote:
> But `ObjId` is `uint64_t`?
I  am preparing an update where I use uint64_t more consistenly.


================
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));
----------------
lebedev.ri wrote:
> `std::tie(<...>) == std::tie(<...>)` ?
You mean: std::forward_as_tuple ?




================
Comment at: llvm/lib/IR/IRBuilder.cpp:514
+IRBuilderBase::CreateNoAliasCopyGuard(Value *BasePtr, Value *NoAliasDecl,
+                                      ArrayRef<long long> EncodedIndices,
+                                      MDNode *ScopeTag, const Twine &Name) {
----------------
lebedev.ri wrote:
> Should this be `uint64_t`?
No. It should be int64_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));
+  }
----------------
lebedev.ri wrote:
> Ops.insert(Ops.end(), MDNodes.begin(), MDNodes.end()) ?
That won't work.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:569
+  for (auto *MDV : MDValues) {
+    Ops.push_back(MDV);
+  }
----------------
lebedev.ri wrote:
> same
But here it will.


================
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");
----------------
lebedev.ri wrote:
> This should be in the parent patch that added that
Maybe. I tried to not do that kind of changes in the rebased versions.


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

https://reviews.llvm.org/D68491