Skip to content

[SYCL] Simplify arguments to computeModuleProperties #15271

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

Merged
merged 1 commit into from
Sep 10, 2024
Merged

[SYCL] Simplify arguments to computeModuleProperties #15271

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
4 changes: 1 addition & 3 deletions llvm/include/llvm/SYCLLowerIR/ComputeModuleRuntimeInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ using EntryPointSet = SetVector<Function *>;

PropSetRegTy computeModuleProperties(const Module &M,
const EntryPointSet &EntryPoints,
const GlobalBinImageProps &GlobProps,
bool SpecConstsMet,
bool IsSpecConstantDefault);
const GlobalBinImageProps &GlobProps);

std::string computeModuleSymbolTable(const Module &M,
const EntryPointSet &EntryPoints);
Expand Down
10 changes: 10 additions & 0 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,6 +72,16 @@ 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[] =
"sycl.specialization-constants-default-values-module";

private:
HandlingMode Mode;
};
Expand Down
13 changes: 9 additions & 4 deletions llvm/lib/SYCLLowerIR/ComputeModuleRuntimeInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,7 @@ uint32_t getKernelWorkGroupNumDim(const Function &Func) {

PropSetRegTy computeModuleProperties(const Module &M,
const EntryPointSet &EntryPoints,
const GlobalBinImageProps &GlobProps,
bool SpecConstsMet,
bool IsSpecConstantDefault) {
const GlobalBinImageProps &GlobProps) {

PropSetRegTy PropSet;
{
Expand All @@ -152,6 +150,10 @@ 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;
Comment on lines +153 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Both collectSpecConstantMetadata and collectSpecConstantDefaultValuesMetadata perform this metadata check:

llvm/llvm/lib/SYCLLowerIR/SpecConstants.cpp

Lines 1035 to 1039 in 9723157

bool SpecConstantsPass::collectSpecConstantMetadata(const Module &M,
SpecIDMapTy &IDMap) {
NamedMDNode *MD = M.getNamedMetadata(SPEC_CONST_MD_STRING);
if (!MD)
return false;

Therefore, I wonder if we just need to omit it all together and instead put PropSet.add under condition that TmpSpecIDMap is not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, ill try this and make a follow-up PR if it works

if (SpecConstsMet) {
// extract spec constant maps per each module
SpecIDMapTy TmpSpecIDMap;
Expand Down Expand Up @@ -369,7 +371,10 @@ PropSetRegTy computeModuleProperties(const Module &M,
if (!HostPipePropertyMap.empty()) {
PropSet.add(PropSetRegTy::SYCL_HOST_PIPES, HostPipePropertyMap);
}

bool IsSpecConstantDefault =
M.getNamedMetadata(
SpecConstantsPass::SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING) !=
nullptr;
if (IsSpecConstantDefault)
PropSet.add(PropSetRegTy::SYCL_MISC_PROP, "specConstsReplacedWithDefault",
1);
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,9 +47,6 @@ 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 @@ -1029,6 +1026,9 @@ PreservedAnalyses SpecConstantsPass::run(Module &M,
for (const auto &P : DefaultsMetadata)
MDDefaults->addOperand(P);

if (Mode == HandlingMode::default_values)
Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually, HandlingMode is both set outside of the pass and read outside of the pass (that property to indicate that a device image was produced with default values of spec constants), so it seems to me that that metadata is better to be set somewhere outside the pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i debated this, let me try putting it somewhere when we do the default split, will do in follow-up pr thx

M.getOrInsertNamedMetadata(SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING);

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

Expand Down
1 change: 1 addition & 0 deletions llvm/test/tools/sycl-post-link/spec-constants/default-value/bool.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

; CHECK: %bool1 = trunc i8 1 to i1
; CHECK: %frombool = zext i1 %bool1 to i8
; CHECK: !sycl.specialization-constants-default-values-module = !{}
; CHECK-LOG: sycl.specialization-constants
; CHECK-LOG:[[UNIQUE_PREFIX:[0-9a-zA-Z]+]]={0, 0, 1}
; CHECK-LOG: sycl.specialization-constants-default-values
Expand Down
5 changes: 2 additions & 3 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 @@ -307,9 +307,8 @@ std::string saveModuleIR(Module &M, int I, StringRef Suff) {
std::string saveModuleProperties(module_split::ModuleDesc &MD,
const GlobalBinImageProps &GlobProps, int I,
StringRef Suff, StringRef Target = "") {
auto PropSet = computeModuleProperties(MD.getModule(), MD.entries(),
GlobProps, MD.Props.SpecConstsMet,
MD.isSpecConstantDefault());
auto PropSet =
computeModuleProperties(MD.getModule(), MD.entries(), GlobProps);

std::string NewSuff = Suff.str();
if (!Target.empty()) {
Expand Down
Loading