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][CodeGen] Special treatment of 3-element extended vector load and store #674

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

seven-mile
Copy link
Collaborator

Continue the work of #613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This PR enable these paths.

Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

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

Could you add a test (or file an issue) for arrays of 3-component vectors, please?

// CIR-NEXT: cir.store %[[#RESULT]], %[[#PVECC]] : !cir.vector<!s32i x 3>, !cir.ptr<!cir.vector<!s32i x 3>>

// LLVM-NEXT: %[[#VECB:]] = load <2 x i32>, ptr %[[#PVECB]], align 8
// LLVM-NEXT: %[[#VECC:]] = load <3 x i32>, ptr %[[#PVECC]], align 16
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches clang's codegen, but do we understand why the vector is not loaded as <4 x i32> here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems a mistake from upstream?

Copy link
Member

Choose a reason for hiding this comment

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

Not blocking the review, but I agree with @jopperm, since you're going to be looking at OpenCL it might be good to try to understand why for your own knowledge of how vectors should play out in general.

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.

LGTM with few clarifying questions / comments for future PRs.

// CIR-NEXT: cir.store %[[#RESULT]], %[[#PVECC]] : !cir.vector<!s32i x 3>, !cir.ptr<!cir.vector<!s32i x 3>>

// LLVM-NEXT: %[[#VECB:]] = load <2 x i32>, ptr %[[#PVECB]], align 8
// LLVM-NEXT: %[[#VECC:]] = load <3 x i32>, ptr %[[#PVECC]], align 16
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking the review, but I agree with @jopperm, since you're going to be looking at OpenCL it might be good to try to understand why for your own knowledge of how vectors should play out in general.

clang/test/CIR/CodeGen/vectype-ext.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
@bcardosolopes bcardosolopes merged commit 5e9148f into llvm:main Jun 11, 2024
7 checks passed
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
…d store (llvm#674)

Continue the work of llvm#613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This
PR enable these paths.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
…d store (llvm#674)

Continue the work of llvm#613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This
PR enable these paths.
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…d store (llvm#674)

Continue the work of llvm#613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This
PR enable these paths.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…d store (llvm#674)

Continue the work of llvm#613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This
PR enable these paths.
lanza pushed a commit that referenced this pull request Nov 5, 2024
…d store (#674)

Continue the work of #613 .

Original CodeGen treat vec3 as vec4 to get aligned memory access. This
PR enable these paths.
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