-
Notifications
You must be signed in to change notification settings - Fork 168
[CIR] Added support for __builtin_ia32_psrldqi_byteshift
#1886
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
base: main
Are you sure you want to change the base?
Conversation
|
||
// Perform the shuffle (right shift by inserting zeros from the left) | ||
mlir::Value shuffleResult = | ||
builder.createVecShuffle(loc, byteCast, zero, indices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be something like this?
builder.createVecShuffle(loc, byteCast, zero, indices); | |
cir::VecShuffleOp::create(builder, loc, byteCast, zero, indices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createVecShuffle
is a function in our builder class CIRGenBuilder, and it's already calling cir::VecShuffleOp::create
inside.
What we convert is from builder.create<cir::VecShuffleOp>(loc, byteCast, zero, indices)
to cir::VecShuffleOp::create(builder, loc, byteCast, zero, indices)
but not our builder functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I would leave builder.createVecShuffle(...)
as is?
The only reason I pointed this out, was because on my previous PR, I was told to convert builder.createVecShuffle(...)
to cir::VecShuffleOp::create(...)
: #1883 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the comment, I think it's related to migrating from builder.create<...>
to Op::create
, so it will make it easy for LSP, not our functions (They are already working well with LSP similar to any other functions in the language)
The thread: https://discourse.llvm.org/t/psa-opty-create-now-with-100-more-tab-complete/87339
cc. @tommymcm
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp View the diff from clang-format here.diff --git a/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp b/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
index ac80c75486..0ce64d27bf 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
@@ -168,7 +168,7 @@ static mlir::Value emitX86PSRLDQIByteShift(CIRGenFunction &cgf,
// If psrldq is shifting the vector more than 15 bytes, emit zero.
if (shiftVal >= 16)
- return builder.getZero(loc, resultType);
+ return builder.getZero(loc, resultType);
auto numElts = resultType.getSize() * 8;
assert(numElts % 16 == 0 && "Expected a multiple of 16");
@@ -177,7 +177,7 @@ static mlir::Value emitX86PSRLDQIByteShift(CIRGenFunction &cgf,
// This correlates to the OG CodeGen
// As stated in the OG, 256/512-bit psrldq operates on 128-bit lanes.
- // So we have to make sure we handle it.
+ // So we have to make sure we handle it.
for (unsigned l = 0; l < numElts; l += 16) {
for (unsigned i = 0; i < 16; ++i) {
unsigned idx = i + shiftVal;
@@ -199,7 +199,6 @@ static mlir::Value emitX86PSRLDQIByteShift(CIRGenFunction &cgf,
return builder.createBitcast(shuffleResult, resultType);
}
-
mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned BuiltinID,
const CallExpr *E) {
if (BuiltinID == Builtin::BI__builtin_cpu_is)
|
WIP:
Still need to add tests
Related Issue: #1885