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_vshl_n_v and neon_vshlq_n_v #965

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

ghehg
Copy link
Collaborator

@ghehg ghehg commented Oct 13, 2024

As title, but important step in this PR is to allow CIR ShiftOp to take vector of int type as input type. As result, I added a verifier to ShiftOp with 2 constraints

  1. Input type either all vector or int type. This is consistent with LLVM::ShlOp, vector shift amount is expected.
  2. In the spirit of C99 6.5.7.3, shift amount type must be the same as result type, the if vector type is used. (This is enforced in LLVM lowering for scalar int type).

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 14, 2024

Read for review, but keep it draft as I anticipate manual merging will happen once other pending PR's accepted

@ghehg ghehg marked this pull request as ready for review October 15, 2024 18:48
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.

Thanks! Some minor changes needed

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIRTypes.td Outdated Show resolved Hide resolved
@dkolsen-pgi
Copy link
Collaborator

It was my mistake that I forgot to implement the shift operators for GNU vector types as part of issue #284. I even have a test program for that in my test area. But I must have assumed that BinOp included all the binary operators that I needed to worry about and didn't go looking for other binary operators that were implemented with different CIR ops.

Changing ShiftOp to also handle vector types is the right thing to do.

If this change fixes vector shift operations in user code (it looks like it might, but I'm not certain), then please update the PR description to say that, and please add shift operations to clang/test/CIR/CodeGen/vectype.cpp (and maybe also to clang/test/CIR/Lowering/vectype.cpp).

If this change doesn't fix vector shift operations, then leave this change alone and open an issue about this. Someone, probably me, will finish fixing that in a later PR.

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 17, 2024

It was my mistake that I forgot to implement the shift operators for GNU vector types as part of issue #284. I even have a test program for that in my test area. But I must have assumed that BinOp included all the binary operators that I needed to worry about and didn't go looking for other binary operators that were implemented with different CIR ops.

Changing ShiftOp to also handle vector types is the right thing to do.

If this change fixes vector shift operations in user code (it looks like it might, but I'm not certain), then please update the PR description to say that, and please add shift operations to clang/test/CIR/CodeGen/vectype.cpp (and maybe also to clang/test/CIR/Lowering/vectype.cpp).

If this change doesn't fix vector shift operations, then leave this change alone and open an issue about this. Someone, probably me, will finish fixing that in a later PR.

The change might support user code, I'll try to find user test code to see if this is the case, if this change alone is enough to support user code, I'll just update clang/test/CIR/CodeGen/vectype.cpp and clang/test/CIR/Lowering/vectype.cpp.
Otherwise, I'll open an issue and either you or I can fix it in a separate PR.

@dkolsen-pgi
Copy link
Collaborator

The test case that I wrote (and then forgot about and didn't use) for vector shift is:

typedef int vi4 __attribute__((vector_size(16)));
int main() {
  vi4 a = { 1, 2, 3, 4 };
  vi4 b = { 5, 6, 7, 8 };
  vi4 c = a << b;
  vi4 d = a >> b;
  return c[1] + d[1];
}

You can start with that for your tests of user code.

@ghehg
Copy link
Collaborator Author

ghehg commented Oct 18, 2024

user code support is confirmed. Added user code test cases covering left, right(logic and arith) shifts

vus2 zamt = { 3, 4 };
// CHECK: %{{[0-9]+}} = cir.const #cir.const_vector<[#cir.int<3> : !u16i, #cir.int<4> : !u16i]> : !cir.vector<!u16i x 2>
vus2 zzz = z >> zamt;
// CHECK: %{{[0-9]+}} = cir.shift( right, {{%.*}} : !cir.vector<!u16i x 2>,
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like we have a buggy " right" with an extra space, let me create an issue.

Copy link
Collaborator Author

@ghehg ghehg Oct 18, 2024

Choose a reason for hiding this comment

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

Haha, I noticed that yesterday but not sure that extra space was on purpose as we had existing test case with that

@bcardosolopes bcardosolopes merged commit e50db7b into llvm:main Oct 18, 2024
6 checks passed
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…lvm#965)

As title, but important step in this PR is to allow CIR ShiftOp to take
vector of int type as input type. As result, I added a verifier to
ShiftOp with 2 constraints

1. Input type either all vector or int type. This is consistent with
LLVM::ShlOp, vector shift amount is expected.
2. In the spirit of C99 6.5.7.3, shift amount type must be the same as
result type, the if vector type is used. (This is enforced in LLVM
lowering for scalar int type).
lanza pushed a commit that referenced this pull request Nov 5, 2024
…965)

As title, but important step in this PR is to allow CIR ShiftOp to take
vector of int type as input type. As result, I added a verifier to
ShiftOp with 2 constraints

1. Input type either all vector or int type. This is consistent with
LLVM::ShlOp, vector shift amount is expected.
2. In the spirit of C99 6.5.7.3, shift amount type must be the same as
result type, the if vector type is used. (This is enforced in LLVM
lowering for scalar int type).
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