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 OpenCL Vector Types #613

Merged
merged 10 commits into from
Jun 6, 2024

Conversation

seven-mile
Copy link
Collaborator

@seven-mile seven-mile commented May 16, 2024

Resolve #532 .

Support CIRGen of ExtVectorElementExpr that includes swizzle v.xyx and subscription v.s0.

@seven-mile seven-mile marked this pull request as ready for review May 16, 2024 18:38
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/vectype-cl.cpp Outdated Show resolved Hide resolved
clang/test/CIR/Lowering/vectype-cl.cpp Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/vectype-cl.cpp Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/vectype-cl.cpp Outdated Show resolved Hide resolved
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.

Way to go on the skeleton work and thanks for fixing the testcase, more comments inline.

clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenValue.h Outdated Show resolved Hide resolved
@seven-mile
Copy link
Collaborator Author

Test cases improved. Tell me if I missed something! ; )

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.

(The free-function nits are in reference to https://mlir.llvm.org/deprecation/; please check all occurrences in this PR.)

clang/lib/CIR/CodeGen/CIRGenBuilder.h Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExpr.cpp Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/vectype-ext.cpp Outdated Show resolved Hide resolved
@seven-mile
Copy link
Collaborator Author

Nice catch! I appreciate it. Updated.

Removed test cases for int7 and the code path to support odd-component vectors. Will submit the patch for 3-component vectors and the removed part afterward.

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 after one minor pending inline question! Thanks!

[CIR][NFC] Arrange test cases

[CIR][NFC] Use ArrayAttr for elts

[CIR][NFC] Test store one element and make HLSL NYI

[CIR][NFC] Add test for one element load

line 687 of CIRGenExpr.cpp

[CIR][NFC] Fix untagged values

[CIR][NFC] Add test for extended-sized vec store

for branch: line 755 in CIRGenExpr.cpp

# Conflicts:
#	clang/test/CIR/CodeGen/vectype-ext.cpp

[CIR][NFC] Add test for undefined position in odd-component vectors
// CIR: %{{[0-9]+}} = cir.vec.create(%{{[0-9]+}}, %{{[0-9]+}}, %{{[0-9]+}}, %{{[0-9]+}} : !s32i, !s32i, !s32i, !s32i) : !cir.vector<!s32i x 4>
// LLVM: %[[#X1:]] = load i32, ptr %{{[0-9]+}}, align 4
// LLVM-NEXT: %[[#X2:]] = load i32, ptr %{{[0-9]+}}, align 4
// LLVM-NEXT: %[[#SUM:]] = add i32 %[[#X2]], 1
Copy link
Member

Choose a reason for hiding this comment

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

If we look at how LLVM currently codegen for this, we get a add nsw instead of a plain add, see https://godbolt.org/z/xdevh38r8. There are probably other differences too.

I believe its not related to this specific PR, but to set expectations: whenever LLVM checks are written, that should not be done blindly, we should actually check it against what traditional (OG) codegen currently emits. If there are semantic differences, they ideally should be fixed as part of the PR, but if not, we should create an issue so someone else could take a look later.

I'll create an issue for this one, can you please go through the rest of the file and see if you can spot more differences and create issues for them? Thanks in advance!

Copy link
Member

Choose a reason for hiding this comment

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

Done in #664

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Many thanks for finding out this diff! I actually went through the final LLVM IR and compared them, but missed this overflow flag quite carelessly.

After a double check, I found the default index types used by two paths are different: i64 for CIR and i32 for Clang. But if user is using other types(like vec[x: i32 or i64]), in both paths, they will be preserved as is. This PR chose i64 because the previous works from @dkolsen-pgi have already done it. Since they are all self-consistent, I don't think it's a problem.

Nothing else special.

@bcardosolopes bcardosolopes merged commit a949a65 into llvm:main Jun 6, 2024
6 checks passed
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.

OpenCL vector types
3 participants