Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bcardosolopes opened this issue Oct 30, 2024 · 8 comments
Open

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

bcardosolopes opened this issue Oct 30, 2024 · 8 comments
Labels
good first issue Good for newcomers IR difference A difference in ClangIR-generated LLVM IR that could complicate reusing original CodeGen tests

Comments

@bcardosolopes
Copy link
Member

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?

@bcardosolopes bcardosolopes added good first issue Good for newcomers IR difference A difference in ClangIR-generated LLVM IR that could complicate reusing original CodeGen tests labels Oct 30, 2024
@liusy58
Copy link

liusy58 commented Nov 3, 2024

I will spend some time on it..

@liusy58
Copy link

liusy58 commented Nov 21, 2024

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
Copy link

liusy58 commented Nov 21, 2024

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
Copy link
Member Author

@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
Copy link

liusy58 commented Nov 24, 2024

@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
Copy link
Member Author

bcardosolopes commented Nov 25, 2024

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
Copy link

liusy58 commented Nov 26, 2024

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

@bcardosolopes
Copy link
Member Author

The original LLVM codegen in lib/CodeGen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers IR difference A difference in ClangIR-generated LLVM IR that could complicate reusing original CodeGen tests
Projects
None yet
Development

No branches or pull requests

2 participants