[libcxx-commits] [PATCH] D158861: [llvm] Move CallInst::CreateMalloc to IRBuilderBase::CreateMalloc

[Nikita Popov via Phabricator via libcxx-commits]

nikic removed a reviewer: libc++abi.
nikic added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:632
+  CallInst *CreateMalloc(Type *IntPtrTy, Type *AllocTy, Value *AllocSize,
+                         Value *ArraySize, Function *MallocF,
+                         const Twine &Name = "");
----------------
You can also make `MallocF = nullptr` (the default is using `malloc`).


================
Comment at: llvm/lib/IR/Core.cpp:3539
+      unwrap(B)->CreateMalloc(ITy, unwrap(Ty), AllocSize, nullptr, nullptr, "");
   return wrap(unwrap(B)->Insert(Malloc, Twine(Name)));
 }
----------------
nikic wrote:
> This should directly return the CreateMalloc result, as that already performs the insertion. Also pass Name to it.
This is not done yet (you only adjusted the case below).


================
Comment at: llvm/lib/IR/IRBuilder.cpp:295
+/// IsConstantOne - Return true only if val is constant int 1
+static bool IsConstantOne(Value *val) {
+  assert(val && "IsConstantOne does not work with nullptr val");
----------------
IsConstantOne -> isConstantOne
val -> Val

Per current style guide.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:312
+  else if (ArraySize->getType() != IntPtrTy) {
+    ArraySize = CastInst::CreateIntegerCast(ArraySize, IntPtrTy, false, "");
+  }
----------------
Use `CreateIntCast` on IRBuilder to insert the instruction. Or alternatively `CreateZExtOrTrunc` which is a bit more explicit.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:318-320
+    } else if (Constant *CO = dyn_cast<Constant>(ArraySize)) {
+      Constant *Scale =
+          ConstantExpr::getIntegerCast(CO, IntPtrTy, false /*ZExt*/);
----------------
Drop this case -- the one below will handle it.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:345
+    if (!F->returnDoesNotAlias())
+      F->setReturnDoesNotAlias();
+  }
----------------
You can drop the `!F->returnDoesNotAlias()` check (it will be done by setReturnDoesNotAlias).


================
Comment at: llvm/lib/IR/IRBuilder.cpp:325
+      // Multiply type size by the array size...
+      AllocSize = BinaryOperator::CreateMul(ArraySize, AllocSize, "mallocsize");
+    }
----------------
nikic wrote:
> Call IRBuilder's `CreateBinOp()` here, to actually insert the instruction.
Use `CreateMul(` instead of `CreateBinOp(Instruction::Mul,`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp:1297
   // setjmpTable[0] = 0;
   IRB.SetInsertPoint(SetjmpTableSize);
   IRB.CreateStore(IRB.getInt32(0), SetjmpTable);
----------------
This call needs to be moved before the CreateMalloc, as it already needs the insertion point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158861