[SYCL] Add dummy image generation for virtual functions #15942
Conversation
| bool dummyEmitted = false; | ||
| for (module_split::ModuleDesc &IrMD : MMs) { | ||
| if (auto Dummy = makeDummy(IrMD)) { | ||
| saveModule(Tables, *Dummy, ID, OutIRFileName, /*IsDummy*/ true); | ||
| dummyEmitted = true; | ||
| } | ||
| } | ||
| if (dummyEmitted) | ||
| ++ID; |
There was a problem hiding this comment.
We either need some comments here, or the function should be renamed (and likely we need both).
For an unfamiliar reader it could be a mystery what "dummy" means and why do we need to make it for every device image we have produced.
Also: do we have any information about device image carrying virtual function definitions in ModuleDesc, or any of its members? If we had an if like that at this level, it would save us function arguments lookup and would work as an extra documentation piece
| @@ -893,6 +929,16 @@ processInputModule(std::unique_ptr<Module> M) { | |||
|
|
|||
| ++ID; | |||
| } | |||
|
|
|||
| bool dummyEmitted = false; | |||
There was a problem hiding this comment.
| bool dummyEmitted = false; | |
| bool DummyEmitted = false; |
| @@ -741,6 +747,36 @@ bool isTargetCompatibleWithModule(const std::string &Target, | |||
| return true; | |||
| } | |||
|
|
|||
| std::optional<module_split::ModuleDesc> | |||
| makeDummy(module_split::ModuleDesc &MD) { | |||
| bool hasVirtualFunctions = false; | |||
There was a problem hiding this comment.
| bool hasVirtualFunctions = false; | |
| bool HasVirtualFunctions = false; |
| ; CHECK-FP64-DUMMY-NEXT: entry: | ||
| ; CHECK-FP64-DUMMY-NEXT: ret void | ||
|
|
||
| ; CHECK-FP64-DUMMY-PROPS: dummy=1 |
There was a problem hiding this comment.
I would also check that we have not emitted the property for other device images. And I would also throw a virtual function without any optional kernel features on it into the mix as well
| @@ -816,6 +816,15 @@ void ModuleDesc::saveSplitInformationAsMetadata() { | |||
| SpecConstantsPass::SPEC_CONST_DEFAULT_VAL_MODULE_MD_STRING); | |||
| } | |||
|
|
|||
| ModuleDesc ModuleDesc::makeDummy() const { | |||
| ModuleDesc MD(CloneModule(getModule())); | |||
There was a problem hiding this comment.
It looks like we are cloning the module two times: first time here and then during call to ModuleDesc::makeDummyImageIR within saveModuleIR and I don't think that we really need this first clone.
I suggest that we call saveModuleIR here immediately, thus saving a module copy (they are generally expensive).
| @@ -225,6 +226,9 @@ class ModuleDesc { | |||
|
|
|||
| void saveSplitInformationAsMetadata(); | |||
|
|
|||
| ModuleDesc makeDummy() const; | |||
There was a problem hiding this comment.
| ModuleDesc makeDummy() const; | |
| ModuleDesc makeDummyCopy() const; |
To better indicate that it creates a clone. Otherwise the name may suggest that it simply modifies the existing ModuleDesc
| @@ -296,11 +297,35 @@ void saveModuleIR(Module &M, StringRef OutFilename) { | |||
| MPM.run(M, MAM); | |||
| } | |||
|
|
|||
| std::string saveModuleIR(Module &M, int I, StringRef Suff) { | |||
| DUMP_ENTRY_POINTS(M, EmitOnlyKernelsAsEntryPoints, "saving IR"); | |||
| std::unique_ptr<Module> makeDummyImageIR(const Module &M) { | |||
There was a problem hiding this comment.
When we do code split we don't clone the whole module, but we only take what's necessary.
As I understand it, the input here is a device image which contains virtual function definitions. But the code below suggests that it also may contain other functions which are called by virtual functions. If we can save on copying them, then we probably should.
No description provided.