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][Builtin][Neon] Lower neon_vtrn and neon_vtrnq #942

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

ghehg
Copy link
Collaborator

@ghehg ghehg commented Oct 7, 2024

The generated code is the same as Clang codeden except in a small discrepancy when GEP:
OG generates code like this:
%6 = getelementptr inbounds <4 x i16>, ptr %retval.i, i32 1
CIR generates a bit differently:
%6 = getelementptr <4 x i16>, ptr %retval.i, i64 1
Ptr offest might be trivial because choosing i64 over i32 as index type seems to be LLVM Dialect's choice.

The lack of inbounds keyword might be an issue as mlir::cir::PtrStrideOp is currently not lowering to LLVM:GEPOp with inbounds attribute as mlir::cir::PtrStrideOp itself has no inbounds. It's probably because there was no need for it though we do have an implementation of CIRGenFunction::buildCheckedInBoundsGEP .

Anyway, the issue is not in the scope of this PR and should be addressed in a separate PR. If we think this is an issue, I can create another PR and probably add optional attribute to mlir::cir::PtrStrideOp to achieve it.

In addition to lowering work, a couple of more works:

  1. Did a little refactoring on variable name changing into desired CamelBack case.
  2. Changed neon-misc RUN Options to be consistent with other neon test files and make test case more concise.

@ghehg ghehg changed the title Lower neon_vtrn and neon_vtrnq [CIR][CIRGen][Builtin][Neon] Lower neon_vtrn and neon_vtrnq Oct 7, 2024
@ghehg ghehg marked this pull request as ready for review October 7, 2024 22:23
@bcardosolopes
Copy link
Member

@ghehg

Anyway, the issue is not in the scope of this PR and should be addressed in a separate PR

Can you please create an issue to track it? Thanks

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, I'll merge as soon as the issue is created.

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 8, 2024

@ghehg

Anyway, the issue is not in the scope of this PR and should be addressed in a separate PR

Can you please create an issue to track it? Thanks
Just created the issue with a simple sample code.
#952
Assigned it to myself, and will fix it soon

@bcardosolopes
Copy link
Member

There's a conflict needs fixing

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 9, 2024

There's a conflict needs fixing

sure, I'll rebase in my local repo and update the diff

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 9, 2024

There's a conflict needs fixing
conflict should be resolved.

@bcardosolopes
Copy link
Member

now needs rebasing post open source rebase :)

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 10, 2024

now needs rebasing post open source rebase :)

done, this rebase is easy :-)

@bcardosolopes
Copy link
Member

still conflicts, active week

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 11, 2024

still conflicts, active week

yet another rebase, hopefully weekend is quieter.

@bcardosolopes bcardosolopes merged commit 3d43e93 into llvm:main Oct 11, 2024
6 checks passed
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
as title. 
The generated code is the same as Clang codeden except in a small
discrepancy when GEP:
OG generates code like this: 
`%6 = getelementptr inbounds <4 x i16>, ptr %retval.i, i32 1`
CIR generates a bit differently:
`%6 = getelementptr <4 x i16>, ptr %retval.i, i64 1`
Ptr offest might be trivial because choosing i64 over i32 as index type
seems to be LLVM Dialect's choice.

The lack of `inbounds` keyword might be an issue as
`mlir::cir::PtrStrideOp` is currently not lowering to LLVM:GEPOp with
`inbounds` attribute as `mlir::cir::PtrStrideOp` itself has no
`inbounds`. It's probably because there was no need for it though we do
have an implementation of [`CIRGenFunction::buildCheckedInBoundsGEP`
](https://github.com/llvm/clangir/blob/10d6f4b94da7e0181a070f0265d079419d96cf78/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2762).

Anyway, the issue is not in the scope of this PR and should be addressed
in a separate PR. If we think this is an issue, I can create another PR
and probably add optional attribute to `mlir::cir::PtrStrideOp` to
achieve it.

In addition to lowering work, a couple of more works:

1. Did a little refactoring on variable name changing into desired
CamelBack case.
2. Changed neon-misc RUN Options to be consistent with other neon test
files and make test case more concise.
lanza pushed a commit that referenced this pull request Nov 5, 2024
as title. 
The generated code is the same as Clang codeden except in a small
discrepancy when GEP:
OG generates code like this: 
`%6 = getelementptr inbounds <4 x i16>, ptr %retval.i, i32 1`
CIR generates a bit differently:
`%6 = getelementptr <4 x i16>, ptr %retval.i, i64 1`
Ptr offest might be trivial because choosing i64 over i32 as index type
seems to be LLVM Dialect's choice.

The lack of `inbounds` keyword might be an issue as
`mlir::cir::PtrStrideOp` is currently not lowering to LLVM:GEPOp with
`inbounds` attribute as `mlir::cir::PtrStrideOp` itself has no
`inbounds`. It's probably because there was no need for it though we do
have an implementation of [`CIRGenFunction::buildCheckedInBoundsGEP`
](https://github.com/llvm/clangir/blob/10d6f4b94da7e0181a070f0265d079419d96cf78/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2762).

Anyway, the issue is not in the scope of this PR and should be addressed
in a separate PR. If we think this is an issue, I can create another PR
and probably add optional attribute to `mlir::cir::PtrStrideOp` to
achieve it.

In addition to lowering work, a couple of more works:

1. Did a little refactoring on variable name changing into desired
CamelBack case.
2. Changed neon-misc RUN Options to be consistent with other neon test
files and make test case more concise.
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.

2 participants