Skip to content

[CIR][CIRGen][Builtin] Support __builtin_memcpy_inline #1069

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

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

ghehg
Copy link
Collaborator

@ghehg ghehg commented Nov 6, 2024

The test code is from OG's clang/test/CodeGen/builtins-memcpy-inline.c
Also, a little design choice when introducing MemCpyInlineOp, I chose to let it inherit CIR_MemCpyOp, so in future when we optimize MemCpy like Ops, we'd have cleaner and more unified code.
However, the cost is that during LLVM lowering I'd have to convert the length from ConstOp into IntegerAttr as that's what LLVM dialect is expecting

@ghehg ghehg requested a review from smeenai November 6, 2024 15:54
@smeenai
Copy link
Collaborator

smeenai commented Nov 6, 2024

Another potential design here would be to add an inline attribute to the existing memcpy op. What do you think are the pros and cons of that vs. a new attribute?

@ghehg
Copy link
Collaborator Author

ghehg commented Nov 6, 2024

Another potential design here would be to add an inline attribute to the existing memcpy op. What do you think are the pros and cons of that vs. a new attribute?

It is an alternative, and the pro is that we have less CIR Ops and more unified code,
However, Cons are first we still have to do things differently at CodeGen and Lowering (like len must be a ConstOp, and later in LLVM Lowering, convert its value to mlir::IntegerAttr), thus using same CIR Op may make lowering code a bit more complicated.

another small con is the current Op name of "cir.libc.memcpy" would be confusing as in the case of inline, there should never been a call to lib func.

Also, may not be a con necessarily, but
one key difference of __builtin_memcpy_inline is that unlike memcpy, __builtin_memcpy_inline has an additional requirement that:
"Note that the size argument must be a compile time constant."

@ghehg ghehg marked this pull request as ready for review November 6, 2024 19:26
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction looks good.

"Note that the size argument must be a compile time constant."

Looks like you sacrificed using an attribute directly to avoid changing CIR_MemCpyOp? Please use an mlir::IntegerAttr for the length instead of generating a mlir::cir::ConstantOp (the PR is explicitly calling into EvaluateKnownConstInt). You can move the extraClassDeclaration to only the definitions that make sense. Since you have to touch CIR_MemCpyOp anyways, please rename it to CIR_MemOp since in practice it's covering more than memcpys.

@bcardosolopes
Copy link
Member

You can move the extraClassDeclaration to only the definitions that make sense. Since you have to touch CIR_MemCpyOp anyways, please rename it to CIR_MemOp since in practice it's covering more than memcpys.

Seems like you ignored this and didn't explain why you decided not to reuse the class, just refactor as instructed please, I'd prefer if all of these operations use the same class.

@ghehg
Copy link
Collaborator Author

ghehg commented Nov 10, 2024

You can move the extraClassDeclaration to only the definitions that make sense. Since you have to touch CIR_MemCpyOp anyways, please rename it to CIR_MemOp since in practice it's covering more than memcpys.

Seems like you ignored this and didn't explain why you decided not to reuse the class, just refactor as instructed please, I'd prefer if all of these operations use the same class.

Sorry for confusion. I wasn't quite sure that you like to use the class, as a big piece of reusing the class is to have commonArgs and push let arguments down to class hierarchy, also feel uncomfortable pinging you on weekend, so just push the code as a basis for question on Monday.
But now a bit more clear, lmk if this version is aligned with what's you are thinking of.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks for the fast turn-around! LGTM

@bcardosolopes bcardosolopes merged commit d5b2a8e into llvm:main Nov 11, 2024
6 checks passed
lanza pushed a commit that referenced this pull request Mar 18, 2025
The test code is from [OG's
clang/test/CodeGen/builtins-memcpy-inline.c](https://github.com/llvm/clangir/blob/5f1afad625f1292ffcf02c36402d292c46213c86/clang/test/CodeGen/builtins-memcpy-inline.c#L7)
Also, a little design choice when introducing MemCpyInlineOp, I chose to
let it inherit CIR_MemCpyOp, so in future when we optimize MemCpy like
Ops, we'd have cleaner and more unified code.
However, the cost is that during LLVM lowering I'd have to convert the
length from ConstOp into IntegerAttr as that's [what LLVM dialect is
expecting](https://mlir.llvm.org/docs/Dialects/LLVM/#llvmintrmemcpyinline-llvmmemcpyinlineop)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants