[llvm] Move CallInst::CreateMalloc to IRBuilderBase::CreateMalloc

[kwk]

Summary

This removes CreateMalloc from CallInst and adds it to the IRBuilderBase class.

We no longer needed the Instruction *InsertBefore and
BasicBlock *InsertAtEnd arguments of the createMalloc helper
function because we're using IRBuilder now. That's why I we also don't
need 4 CreateMalloc functions, but only two.

[kwk]

llvm/bindings/ocaml/llvm/llvm.mli

2038

@nikic I wonder if I should have kept this. I don't know no *.mli files if this was a binding to CallInst::CreateMalloc, then it should go I think.

[nikic]

The libcxxabi/test/test_demangle.pass.cpp file should not be modified. It uses LLVM symbol names but is completely unrelated.

llvm/bindings/ocaml/llvm/llvm.mli

2038

Yes, this should be kept. You can just adjust the method name in the comment.

llvm/examples/BrainF/BrainF.cpp

97–98

I think you need to drop this line, as CreateMalloc already does the insertion.

llvm/include/llvm/IR/IRBuilder.h

633

We usually do not require a name in IRBuilder.

llvm/lib/IR/Core.cpp

3537–3538

This should directly return the CreateMalloc result, as that already performs the insertion. Also pass Name to it.

llvm/lib/IR/IRBuilder.cpp

308

Drop the bitcasts from this comment, as they're no longer present.

325

Call IRBuilder's CreateBinOp() here, to actually insert the instruction.

332

Use the Context member.

339

Use IRBuilder's CreateCall() to also insert the instruction.

Also pass Name instead of "MCall" here.

[kwk]

Address review comments.

• Undo changes to libcxxabi/test/test_demangle.pass.cpp
• Undo changes to llvm/bindings/ocaml/llvm/llvm.mli
• Change comment from CallInst:: to IRBuilderBase::
• Drop insertion because IRBuilderBase::CreateMalloc already does it
• Don't require a name in IRBuilderBase::CreateMalloc method, as usual
• Directly return value from CreateMalloc and wrap it as an LLVMValueRef
• Drop left over bitcast comment
• Use Context member
• Pass Name instead of "MCall"
• Use IRBuilderBase::CreateCall instead of CallInst::Create
• Call IRBuilder's CreateBinOp() to actually insert the instruction

[nikic]

llvm/include/llvm/IR/IRBuilder.h

632

You can also make MallocF = nullptr (the default is using malloc).

llvm/lib/IR/Core.cpp

3537–3538

This is not done yet (you only adjusted the case below).

llvm/lib/IR/IRBuilder.cpp

295

IsConstantOne -> isConstantOne
val -> Val

Per current style guide.

312

Use CreateIntCast on IRBuilder to insert the instruction. Or alternatively CreateZExtOrTrunc which is a bit more explicit.

318–320

Drop this case -- the one below will handle it.

325

Use CreateMul( instead of CreateBinOp(Instruction::Mul,.

345

You can drop the !F->returnDoesNotAlias() check (it will be done by setReturnDoesNotAlias).

llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp

1303–1304

This call needs to be moved before the CreateMalloc, as it already needs the insertion point.

[kwk]

Address review comments.

• Make MallocF in CreateMalloc optional
• Return CreateMalloc result directly and wrap it as an LLVMValueRef
• Rename static IsConstantOne to isConstantOne
• Use IRBuilder's CreateIntCast instead of CastInst::CreateIntegerCast
• Use IRBuilder's CreateMul instead of CreateBinOp
• Drop returnDoesNotAlias check
• Move SetInsertPoint before CreateMalloc
• remove constant arrayssize case

[nikic]

The code basically looks good to me, but there are a number of test failures. At a glance they may be due to changes to variable names in test output.

llvm/lib/IR/IRBuilder.cpp

311

Omit braces for single-line if.

345

It looks like you also dropped the call to setReturnDoesNotAlias(), not just the check.

llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp

1303

I think this comment should be moved above the IRB.CreateStore(IRB.getInt32(0), SetjmpTable) line to make sense.

[kwk]

• Omit braces for single if-line
• Move comment to where it makes sense
• Re-add F->setReturnDoesNotAlias();

[nikic]

The implementation looks fine to me now, so only the test updates are missing.

[kwk]

Do not change somewhat broken naming behavior.

Before, when CreateMalloc was still part of CallInst a malloc call
was always named "malloccall" unless a bitcast was needed which is
when the Name argument was being used. but only then.
To me this looked like an inconsistency. As a matter of fact, the bitcast
operation is no longer needed, hence I was advised to remove it. Doing so
results in a pretty big change with respect to the tests which all have used
%malloccall´ in their CHECK` lines. To keep up the confidence in my
patch and to keep it simple, I decided to go back to the behavior of
CallInst::CreateMalloc. In a future commit we should drop the bitcast
operation and change the return type of IRBuilderBase::CreateMalloc
from Instruction* to CallInst*.

[kwk]

• Remove unused variable

[kwk]

• Attempt to address failing polly/test/CodeGen/MemAccess/create_arrays_heap.ll

[kwk]

• Revert "Attempt to address failing polly/test/CodeGen/MemAccess/create_arrays_heap.ll"
• Set the insertion point like before

[tbaeder]

llvm/lib/IR/IRBuilder.cpp

295

[kwk]

• Make argument to isConstantOne const

[nikic]

Would dropping the bitcast but hardcoding the "malloccall" name on the call (for now) work? As far as I can tell your current code creates a bitcast without inserting it, which would result in broken IR.

[kwk]

In D158861#4637392, @nikic wrote:

Would dropping the bitcast but hardcoding the "malloccall" name on the call (for now) work? As far as I can tell your current code creates a bitcast without inserting it, which would result in broken IR.

@nikic sorry for not having seen this. I still need to learn a lot about IR. I somehow thought the bitcast was inserted but I never checked. In order to keep the old behaviour for now I would have to insert the bitcast, right? And why would it be broken IR if I'd insert it?

[kwk]

• insert formally uninserted bitcast

[nikic]

I went ahead and dropped the bitcast from the current implementation in https://github.com/llvm/llvm-project/commit/4491f0b969c04c93e8db397d51c6986e9371a49b. That commit contains the necessary test changes. So I think you should be able to revert this to the version without the bitcast and it should "just work" (hopefully).

[kwk]

Rebase

[nikic]

LGTM