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.

Sign up for GitHub

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

Jump to bottom

[SYCL][NFCI] Rework spec constants metadata used for split #15346

Merged
merged 1 commit into from
Sep 11, 2024
Merged

[SYCL][NFCI] Rework spec constants metadata used for split #15346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions llvm/include/llvm/SYCLLowerIR/SpecConstants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,6 @@ class SpecConstantsPass : public PassInfoMixin<SpecConstantsPass> {
collectSpecConstantDefaultValuesMetadata(const Module &M,
std::vector<char> &DefaultValues);

// Name of the metadata which holds a list of all specialization constants
// (with associated information) encountered in the module
static constexpr char SPEC_CONST_MD_STRING[] =
"sycl.specialization-constants";

// Name of the metadata which indicates this module was proccessed with the
// default values handing mode.
static constexpr char SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING[] =
Expand Down
13 changes: 5 additions & 8 deletions llvm/lib/SYCLLowerIR/ComputeModuleRuntimeInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,11 @@ PropSetRegTy computeModuleProperties(const Module &M,
PropSet.add(PropSetRegTy::SYCL_DEVICE_REQUIREMENTS,
computeDeviceRequirements(M, EntryPoints).asMap());
}
auto *SpecConstsMD =
M.getNamedMetadata(SpecConstantsPass::SPEC_CONST_MD_STRING);
bool SpecConstsMet =
SpecConstsMD != nullptr && SpecConstsMD->getNumOperands() != 0;
if (SpecConstsMet) {
// extract spec constant maps per each module
SpecIDMapTy TmpSpecIDMap;
SpecConstantsPass::collectSpecConstantMetadata(M, TmpSpecIDMap);

// extract spec constant maps per each module
SpecIDMapTy TmpSpecIDMap;
SpecConstantsPass::collectSpecConstantMetadata(M, TmpSpecIDMap);
if (!TmpSpecIDMap.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that both spec constants metadata and default values metadata are in sync, i.e. if one is empty then another is also empty. However, I wonder if we need to note that in a comment, or in form of some assert in else branch put under #ifndef NDEBUG?

I'm just looking for what others think, I do not have a strong opinion here, so not asking to do any changes just yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable to me, let me add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running unit tests, it seems it actually is the case that TmpSpecIDMap is empty but IsSpecConstantDefault is true, and I'm not sure if there are any other cases that should bet rue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging for now, let me know if you have any other ideas on something we can assert, happy to implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Running unit tests, it seems it actually is the case that TmpSpecIDMap is empty but IsSpecConstantDefault is true, and I'm not sure if there are any other cases that should bet rue.

What I meant is that collectSpecConstantMetadata and collectSpecConstantMetadata should either both return empty map, or should both return non-empty map. That's the implicit expectation we currently have.

I'm not sure what IsSpecConstantDefault is, but if that's a marker that device image contains default values of spec constants then it is also a weird state. That image should not be produced if there are no spec constants, because it will be no different from the original device image it was produced from. We even have an early exit in the corresponding function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry the two functions you mentioned are the same, did you mean collectSpecConstantMetadata and collectSpecConstantDefaultValuesMetadata?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, copy-paste mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that seems to work, pr coming soon. thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#15420

PropSet.add(PropSetRegTy::SYCL_SPECIALIZATION_CONSTANTS, TmpSpecIDMap);

// Add property with the default values of spec constants
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/SYCLLowerIR/SpecConstants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ constexpr char SPIRV_GET_SPEC_CONST_VAL[] = "__spirv_SpecConstant";
constexpr char SPIRV_GET_SPEC_CONST_COMPOSITE[] =
"__spirv_SpecConstantComposite";

// Name of the metadata which holds a list of all specialization constants (with
// associated information) encountered in the module
constexpr char SPEC_CONST_MD_STRING[] = "sycl.specialization-constants";
// Name of the metadata which holds a default value list of all specialization
// constants encountered in the module
constexpr char SPEC_CONST_DEFAULT_VAL_MD_STRING[] =
Expand Down Expand Up @@ -1026,9 +1029,6 @@ PreservedAnalyses SpecConstantsPass::run(Module &M,
for (const auto &P : DefaultsMetadata)
MDDefaults->addOperand(P);

if (Mode == HandlingMode::default_values)
M.getOrInsertNamedMetadata(SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING);

return IRModified ? PreservedAnalyses::none() : PreservedAnalyses::all();
}

Expand Down
4 changes: 4 additions & 0 deletions llvm/tools/sycl-post-link/sycl-post-link.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,10 @@ processSpecConstantsWithDefaultValues(const module_split::ModuleDesc &MD) {
assert(NewModuleDesc->Props.SpecConstsMet &&
"This property should be true since the presence of SpecConsts "
"has been checked before the run of the pass");
// Add metadata to the module so we can identify it as the default value split
// later.
NewModuleDesc->getModule().getOrInsertNamedMetadata(
SpecConstantsPass::SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING);
NewModuleDesc->rebuildEntryPoints();
return NewModuleDesc;
}
Expand Down
Loading