-
Notifications
You must be signed in to change notification settings - Fork 736
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
[NFC][SYCL][SYCLLowerIR] Add cl::opt SpecConstantMode for LIT testing #15821
[NFC][SYCL][SYCLLowerIR] Add cl::opt SpecConstantMode for LIT testing #15821
Conversation
This allows testing SpecConstantsPass with different modes.
3 failed sycl e2e tests in
|
@@ -29,6 +29,19 @@ | |||
|
|||
using namespace llvm; | |||
|
|||
static cl::opt<SpecConstantsPass::HandlingMode> SpecConstantMode( | |||
"spec-constant-mode", cl::Optional, cl::Hidden, | |||
cl::desc("Spec constant handling mode"), |
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.
cl::desc("Spec constant handling mode"), | |
cl::desc("Specialization constant handling mode"), |
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.
done
cl::values( | ||
clEnumValN(SpecConstantsPass::HandlingMode::default_values, | ||
"default_values", | ||
"spec constant uses are replaced by default values"), |
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.
"spec constant uses are replaced by default values"), | |
"Specialization constant uses are replaced by default values"), |
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.
done
"default_values", | ||
"spec constant uses are replaced by default values"), | ||
clEnumValN(SpecConstantsPass::HandlingMode::emulation, "emulation", | ||
"spec constant intrinsics are replaced by RT buffers"), |
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.
"spec constant intrinsics are replaced by RT buffers"), | |
"Specialization constant intrinsics are replaced by run-time buffers"), |
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.
done
"spec constant intrinsics are replaced by RT buffers"), | ||
clEnumValN( | ||
SpecConstantsPass::HandlingMode::native, "native", | ||
"spec constant intrinsics are lowered to spirv intrinsics"))); |
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.
"spec constant intrinsics are lowered to spirv intrinsics"))); | |
"Specialization constant intrinsics are lowered to SPIR-V intrinsics"))); |
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.
done
@@ -1,7 +1,6 @@ | |||
; RUN: sycl-post-link -properties -split=auto -spec-const=native -S -o %t.table %s -generate-device-image-default-spec-consts |
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.
Is the sycl-post-link test not expected to work anymore? I somehow feel it will be better to add a new test here.
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.
sycl-post-link test is still expected to work.
This test was only checking output of SpecConstantPass, while other tests in llvm/test/tools/sycl-post-link/ folder are also checking sycl-post-link tool output. So I move this test into llvm/test/SYCLLowerIR/SpecConstants/SYCL-alloca.ll
After reading your comment, I also feel it is better to keep the test to check sycl-post-link is correctly passing parameters to SpecConstantPass. So I added it back in the second commit.
@@ -29,6 +29,19 @@ | |||
|
|||
using namespace llvm; | |||
|
|||
static cl::opt<SpecConstantsPass::HandlingMode> SpecConstantMode( |
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.
is there a default value that can be set here?
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.
added default value: cl::init(SpecConstantsPass::HandlingMode::emulation)
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.
Looks good overall. Just a few suggestions.
Thanks
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.
Thanks for the updates. LGTM.
@intel/llvm-gatekeepers please merge, thanks |
This allows testing SpecConstantsPass with different modes.