Use @llvm.memcpy.p0.p0.i64 instead of @llvm.memcpy.p0.p0.i32

[bcardosolopes]

See clang/test/CIR/Lowering/store-memcpy.cpp.

Brief search in mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td indicates there isn't a way to get it via the default operation, but I see mlir/test/Target/LLVMIR/omptarget-depend.mlir generates it, perhaps they generate a direct call?

[liusy58]

I will spend some time on it..

[liusy58]

I found the error..

class CIRCopyOpLowering : public mlir::OpConversionPattern<cir::CopyOp> {
public:
  using mlir::OpConversionPattern<cir::CopyOp>::OpConversionPattern;

  mlir::LogicalResult
  matchAndRewrite(cir::CopyOp op, OpAdaptor adaptor,
                  mlir::ConversionPatternRewriter &rewriter) const override {
    const mlir::Value length = rewriter.create<mlir::LLVM::ConstantOp>(
        op.getLoc(), rewriter.getI32Type(), op.getLength());
    rewriter.replaceOpWithNewOp<mlir::LLVM::MemcpyOp>(
        op, adaptor.getDst(), adaptor.getSrc(), length, op.getIsVolatile());
    return mlir::success();
  }
};

rewriter.getI32Type() directly define the type as int32.

[liusy58]

Not sure that using rewriter.getI64Type() will solve this issue without leading to other errors. But I will try to run all tests later.

[bcardosolopes]

@liusy58 good point, perhaps passing the type as i64 is enough to trigger the right type (we probably just need to figure out a rule of which one to use based on the CIR type in question)

[liusy58]

@bcardosolopes Sorry for my late response. I don't understand what you mean by saying we probably just need to figure out a rule of which one to use based on the CIR type in question. Could you please give more details?

[bcardosolopes]

Could you please give more details?

My point is that sometimes it's not clear if we should generate i32 or i64 (e.g. for arrays it might use i32 and for structs i64, or the other way around - I don't remember, just one possible example). It's possible OG codegen does something that doesn't rely on the original type to decide, we'll need to figure out if right now we already have all the information to emit the right size or if we need to add more information to CIR operations. Does that make sense?

[liusy58]

Thank you. I will check later. By the way, what is OG codegen?

[bcardosolopes]

The original LLVM codegen in lib/CodeGen

[liusy58]

Thank you! Sorry for my slow response. But I will try to figure out this issue.

[bcardosolopes]

@liusy58 assigned a few of the issues to you just to keep track, feel free to remove it!

[liusy58]

in LLVMIntrinsicConversions.inc (which is generated by llvm I guess?)

if (auto op = dyn_cast<::mlir::LLVM::MemcpyOp>(opInst)) {

    auto *inst = LLVM::detail::createIntrinsicCall(
      builder, moduleTranslation, &opInst, llvm::Intrinsic::memcpy,0,{},{0, 1, 2},{3},{StringLiteral("isVolatile")});
    (void) inst;
    
    moduleTranslation.setAccessGroupsMetadata(op, inst);
  
    moduleTranslation.setAliasScopeMetadata(op, inst);
    moduleTranslation.setTBAAMetadata(op, inst);
  
  return success();
}

llvm.memcpy.p0.p0.i32 is generated by combing the types of operands https://github.com/llvm/clangir/blob/main/llvm/lib/IR/Intrinsics.cpp#L168.
As we said before, rewriter.getI32Type() directly define the type as int32.

I try to use rewriter.getI64Type(), but some tests will fail.

[bcardosolopes]

I try to use rewriter.getI64Type(), but some tests will fail.

Possible reasons:

• They fail because they were wrong and testing the wrong thing (i.e. they were not matching OG). If that's the case, just change the test to check for what it now should.
• They fail because you need to add more logic to account for corner cases.

You seem to be going the right direction though :)