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

[CIR][CIRGen] Support __builtin_memset_inline #1114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PikachuHyA
Copy link
Collaborator

No description provided.

support `llvm.intr.memset.inline` in llvm-project repo before we add
support for `__builtin_memset_inline` in clangir

cc @bcardosolopes
@PikachuHyA
Copy link
Collaborator Author

This patch depends on the upstream patch from llvm/llvm-project#115711.

We can proceed with this PR once the ClangIR has been rebased.

@ghehg might be interested in these intrinsics, so please feel free to share your thoughts!

@ghehg ghehg self-requested a review November 13, 2024 13:34
@ghehg
Copy link
Collaborator

ghehg commented Nov 13, 2024

LGTM, Thanks for working on this, especially for adding LLVM::MemsetInlineOp

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.

This is great, I'd suggest you break this PR into everything but LLVM lowering (you can emit a op.emitError() for now) so we land CIRGen faster, once we rebase you add the lowering bits, how does that sound? @lanza is out on vacation, so I don't expect any rebase in the next two weeks (cc @smeenai)

@@ -145,3 +145,23 @@ void test_memcpy_inline_aligned_buffers(unsigned long long *dst, const unsigned
// COM: LLVM: call void @llvm.memcpy.inline.p0.p0.i64(ptr align 8 {{%.*}}, ptr align 8 {{%.*}}, i64 4, i1 false)
__builtin_memcpy_inline(dst, src, 4);
}

void test_memset_inline(void *dst, int val) {
Copy link
Member

@bcardosolopes bcardosolopes Nov 13, 2024

Choose a reason for hiding this comment

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

Following what I said in the review comment: you could wrap this function with #ifndef LLVM_SUPPORT and pass a -DLLVM_SUPPORT to the -emit-llvm RUN line for now. You could leave the LLVM checks in place, just rename LLVM to TODO or something that won't trigger filecheck

@smeenai
Copy link
Collaborator

smeenai commented Nov 13, 2024

If it cherry-picks cleanly, we could also potentially cherry-pick llvm/llvm-project#115711, and it'll just get rebased out on the next rebase.

@bcardosolopes
Copy link
Member

bcardosolopes commented Nov 13, 2024

If it cherry-picks cleanly, we could also potentially cherry-pick llvm/llvm-project#115711, and it'll just get rebased out on the next rebase.

I'm ok with that too, good point. Should we tag the title in any specific way? @PikachuHyA if you want to cherry-pick the dep, put a separate PR for that one and we land this after that!

@smeenai
Copy link
Collaborator

smeenai commented Nov 13, 2024

If it cherry-picks cleanly, we could also potentially cherry-pick llvm/llvm-project#115711, and it'll just get rebased out on the next rebase.

I'm ok with that too, good point. Should we tag the title in any specific way? @PikachuHyA if you want to cherry-pick the dep, put a separate PR for that one and we land this after that!

Maybe [upstream] or [cherry-pick], plus using git cherry-pick -x so that the commit message includes the original hash.

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.

4 participants