Skip to content

Commit fdec8e0

Browse files
kbobrovskbobrovs
committed
address review comments
1 parent e207813 commit fdec8e0

File tree

3 files changed

+43
-36
lines changed

3 files changed

+43
-36
lines changed

‎llvm/tools/sycl-post-link/ModuleSplitter.cpp

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -427,13 +427,13 @@ void ModuleSplitterBase::verifyNoCrossModuleDeviceGlobalUsage() {
427427

428428
#ifndef _NDEBUG
429429

430-
const char *toString(ESIMDStatus S) {
430+
const char *toString(SyclEsimdSplitStatus S) {
431431
switch (S) {
432-
case ESIMDStatus::ESIMD_ONLY:
432+
case SyclEsimdSplitStatus::ESIMD_ONLY:
433433
return "ESIMD_ONLY";
434-
case ESIMDStatus::SYCL_ONLY:
434+
case SyclEsimdSplitStatus::SYCL_ONLY:
435435
return "SYCL_ONLY";
436-
case ESIMDStatus::SYCL_AND_ESIMD:
436+
case SyclEsimdSplitStatus::SYCL_AND_ESIMD:
437437
return "SYCL_AND_ESIMD";
438438
}
439439
return "<UNKNOWN_STATUS>";
@@ -508,11 +508,11 @@ void ModuleDesc::renameDuplicatesOf(const Module &MA, StringRef Suff) {
508508

509509
void ModuleDesc::assignESIMDProperty() {
510510
if (EntryPoints.isEsimd()) {
511-
Props.HasEsimd = ESIMDStatus::ESIMD_ONLY;
511+
Props.HasEsimd = SyclEsimdSplitStatus::ESIMD_ONLY;
512512
} else if (EntryPoints.isSycl()) {
513-
Props.HasEsimd = ESIMDStatus::SYCL_ONLY;
513+
Props.HasEsimd = SyclEsimdSplitStatus::SYCL_ONLY;
514514
} else {
515-
Props.HasEsimd = ESIMDStatus::SYCL_AND_ESIMD;
515+
Props.HasEsimd = SyclEsimdSplitStatus::SYCL_AND_ESIMD;
516516
}
517517
#ifndef _NDEBUG
518518
verifyESIMDProperty();
@@ -521,18 +521,18 @@ void ModuleDesc::assignESIMDProperty() {
521521

522522
#ifndef _NDEBUG
523523
void ModuleDesc::verifyESIMDProperty() const {
524-
if (Props.HasEsimd == ESIMDStatus::SYCL_AND_ESIMD) {
524+
if (Props.HasEsimd == SyclEsimdSplitStatus::SYCL_AND_ESIMD) {
525525
return; // nothing to verify
526526
}
527527
// Verify entry points:
528528
for (const auto *F : entries()) {
529529
const bool IsESIMDFunction = isESIMDFunction(*F);
530530

531531
switch (Props.HasEsimd) {
532-
case ESIMDStatus::ESIMD_ONLY:
532+
case SyclEsimdSplitStatus::ESIMD_ONLY:
533533
assert(IsESIMDFunction);
534534
break;
535-
case ESIMDStatus::SYCL_ONLY:
535+
case SyclEsimdSplitStatus::SYCL_ONLY:
536536
assert(!IsESIMDFunction);
537537
break;
538538
default:
@@ -544,7 +544,7 @@ void ModuleDesc::verifyESIMDProperty() const {
544544
// created to wrap (call) an ESIMD kernel definition, it is not marked with
545545
// "sycl_explicit_simd" attribute in the API headers. Thus it leads to extra
546546
// "SYCL only" module during split. This existed before and needs to be fixed.
547-
// if (Props.HasEsimd == ESIMDStatus::SYCL_ONLY) {
547+
// if (Props.HasEsimd == SyclEsimdSplitStatus::SYCL_ONLY) {
548548
// for (const auto &F : getModule()) {
549549
// assert(!isESIMDFunction(F));
550550
// }

‎llvm/tools/sycl-post-link/ModuleSplitter.h

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ enum IRSplitMode {
3636
// A vector that contains all entry point functions in a split module.
3737
using EntryPointVec = std::vector<const Function *>;
3838

39+
// Represents a named group of device code entry points - kernels and
40+
// SYCL_EXTERNAL functions. There are special group names - "<SYCL>" and
41+
// "<ESIMD>" - which have effect on group processing.
42+
3943
struct EntryPointGroup {
4044
StringRef GroupId;
4145

@@ -47,7 +51,9 @@ struct EntryPointGroup {
4751
: GroupId(GroupId), Functions(std::move(Functions)) {}
4852

4953
const StringRef getId() const { return GroupId; }
54+
// Tells if this group has only ESIMD entry points (based on GroupId).
5055
bool isEsimd() const;
56+
// Tells if this group has only SYCL entry points (based on GroupId).
5157
bool isSycl() const;
5258
void saveNames(std::vector<std::string> &Dest) const;
5359
void rebuildFromNames(const std::vector<std::string> &Names, const Module &M);
@@ -56,15 +62,15 @@ struct EntryPointGroup {
5662

5763
using EntryPointGroupVec = std::vector<EntryPointGroup>;
5864

59-
enum class ESIMDStatus { SYCL_ONLY, ESIMD_ONLY, SYCL_AND_ESIMD };
65+
enum class SyclEsimdSplitStatus { SYCL_ONLY, ESIMD_ONLY, SYCL_AND_ESIMD };
6066

6167
class ModuleDesc {
6268
std::unique_ptr<Module> M;
6369
EntryPointGroup EntryPoints;
6470

6571
public:
6672
struct Properties {
67-
ESIMDStatus HasEsimd = ESIMDStatus::SYCL_AND_ESIMD;
73+
SyclEsimdSplitStatus HasEsimd = SyclEsimdSplitStatus::SYCL_AND_ESIMD;
6874
bool SpecConstsMet = true;
6975
};
7076
std::string Name = "";
@@ -80,8 +86,12 @@ class ModuleDesc {
8086
rebuildEntryPoints(Names);
8187
}
8288

83-
bool isESIMD() const { return Props.HasEsimd == ESIMDStatus::ESIMD_ONLY; }
84-
bool isSYCL() const { return Props.HasEsimd == ESIMDStatus::SYCL_ONLY; }
89+
bool isESIMD() const {
90+
return Props.HasEsimd == SyclEsimdSplitStatus::ESIMD_ONLY;
91+
}
92+
bool isSYCL() const {
93+
return Props.HasEsimd == SyclEsimdSplitStatus::SYCL_ONLY;
94+
}
8595
const EntryPointVec &entries() const { return EntryPoints.Functions; }
8696
EntryPointVec &entries() { return EntryPoints.Functions; }
8797
Module &getModule() { return *M; }
@@ -105,9 +115,9 @@ class ModuleDesc {
105115

106116
void renameDuplicatesOf(const Module &M, StringRef Suff);
107117

108-
// Updates Props.HasEsimd to ESIMDStatus::ESIMD_ONLY/SYCL_ONLY if this module
109-
// descriptor is a ESIMD/SYCL part of the ESIMD/SYCL module split. Otherwise
110-
// assumes the module has both SYCL and ESIMD.
118+
// Updates Props.HasEsimd to SyclEsimdSplitStatus::ESIMD_ONLY/SYCL_ONLY if
119+
// this module descriptor is a ESIMD/SYCL part of the ESIMD/SYCL module split.
120+
// Otherwise assumes the module has both SYCL and ESIMD.
111121
void assignESIMDProperty();
112122
#ifndef _NDEBUG
113123
void verifyESIMDProperty() const;

‎llvm/tools/sycl-post-link/sycl-post-link.cpp

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ struct GlobalBinImageProps {
204204
bool EmitDeviceGlobalPropSet;
205205
};
206206

207-
struct IrPropSymFilenameTripple {
207+
struct IrPropSymFilenameTriple {
208208
std::string Ir;
209209
std::string Prop;
210210
std::string Sym;
@@ -306,7 +306,7 @@ std::vector<uint32_t> getKernelReqdWorkGroupSizeMetadata(const Function &Func) {
306306
}
307307

308308
// Creates a filename based on current output filename, given extension,
309-
// sequential ID and suffix. If the sequential ID I is negative, it is
309+
// sequential ID and suffix.
310310
std::string makeResultFileName(Twine Ext, int I, StringRef Suffix) {
311311
const StringRef Dir0 = OutputDir.getNumOccurrences() > 0
312312
? OutputDir
@@ -423,7 +423,7 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD,
423423
ProgramMetadata.insert({MetadataNames.back(), KernelReqdWorkGroupSize});
424424
}
425425
}
426-
if (MD.Props.HasEsimd == module_split::ESIMDStatus::ESIMD_ONLY) {
426+
if (MD.isESIMD()) {
427427
PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({"isEsimdImage", true});
428428
}
429429
{
@@ -442,6 +442,7 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD,
442442
std::error_code EC;
443443
std::string SCFile = makeResultFileName(".prop", I, Suff);
444444
raw_fd_ostream SCOut(SCFile, EC);
445+
checkError(EC, "error opening file '" + SCFile + "'");
445446
PropSet.write(SCOut);
446447

447448
return SCFile;
@@ -513,17 +514,15 @@ bool lowerEsimdConstructs(module_split::ModuleDesc &MD) {
513514
}
514515

515516
// @param MD Module descriptor to save
516-
// @param IRFilename filename of already avaialble IR component. If not empty,
517-
// IR component saving is skipped, and this file name as recorded as such in
517+
// @param IRFilename filename of already available IR component. If not empty,
518+
// IR component saving is skipped, and this file name is recorded as such in
518519
// the result.
519520
// @return a triple of files where IR, Property and Symbols components of the
520521
// Module descriptor are written respectively.
521-
IrPropSymFilenameTripple saveModule(module_split::ModuleDesc &MD, int I,
522-
StringRef IRFilename = "") {
523-
IrPropSymFilenameTripple Res;
524-
StringRef Suffix = MD.Props.HasEsimd == module_split::ESIMDStatus::ESIMD_ONLY
525-
? "_esimd"
526-
: "";
522+
IrPropSymFilenameTriple saveModule(module_split::ModuleDesc &MD, int I,
523+
StringRef IRFilename = "") {
524+
IrPropSymFilenameTriple Res;
525+
StringRef Suffix = MD.isESIMD() ? "_esimd" : "";
527526

528527
if (!IRFilename.empty()) {
529528
// don't save IR, just record the filename
@@ -556,7 +555,7 @@ module_split::ModuleDesc link(module_split::ModuleDesc &&MD1,
556555
module_split::ModuleDesc Res(MD1.releaseModulePtr(), std::move(Names));
557556
Res.Props.HasEsimd = MD1.Props.HasEsimd == MD2.Props.HasEsimd
558557
? MD1.Props.HasEsimd
559-
: module_split::ESIMDStatus::SYCL_AND_ESIMD;
558+
: module_split::SyclEsimdSplitStatus::SYCL_AND_ESIMD;
560559
Res.Props.SpecConstsMet = MD1.Props.SpecConstsMet || MD2.Props.SpecConstsMet;
561560
Res.Name = "linked[" + MD1.Name + "," + MD1.Name + "]";
562561
return Res;
@@ -603,7 +602,7 @@ bool processCompileTimeProperties(module_split::ModuleDesc &MD) {
603602
constexpr int MAX_COLUMNS_IN_FILE_TABLE = 3;
604603

605604
void addTableRow(util::SimpleTable &Table,
606-
const IrPropSymFilenameTripple &RowData) {
605+
const IrPropSymFilenameTriple &RowData) {
607606
SmallVector<StringRef, MAX_COLUMNS_IN_FILE_TABLE> Row;
608607

609608
for (const std::string *S : {&RowData.Ir, &RowData.Prop, &RowData.Sym}) {
@@ -737,10 +736,8 @@ processInputModule(std::unique_ptr<Module> M) {
737736
Modified |= processSpecConstants(MDesc1);
738737
Modified |= processCompileTimeProperties(MDesc1);
739738

740-
if ((MDesc1.Props.HasEsimd != module_split::ESIMDStatus::SYCL_ONLY) &&
741-
LowerEsimd) {
742-
assert(MDesc1.Props.HasEsimd == module_split::ESIMDStatus::ESIMD_ONLY &&
743-
"NYI");
739+
if (!MDesc1.isSYCL() && LowerEsimd) {
740+
assert(MDesc1.isESIMD() && "NYI");
744741
Modified |= lowerEsimdConstructs(MDesc1);
745742
}
746743
MMs.emplace_back(std::move(MDesc1));
@@ -779,7 +776,7 @@ processInputModule(std::unique_ptr<Module> M) {
779776
"have been made\n";
780777
}
781778
for (module_split::ModuleDesc &IrMD : MMs) {
782-
IrPropSymFilenameTripple T = saveModule(IrMD, ID, OutIRFileName);
779+
IrPropSymFilenameTriple T = saveModule(IrMD, ID, OutIRFileName);
783780
addTableRow(*Table, T);
784781
}
785782
++ID;

0 commit comments

Comments
 (0)