[SYCL] Do not emit unneeded static initializations in sycl device code #1774

premanandrao · 2020-05-28T16:00:13Z

Signed-off-by: Premanand M Rao [email protected]

erichkeane · 2020-05-28T16:05:34Z

clang/lib/CodeGen/CodeGenModule.cpp

@@ -4687,6 +4687,10 @@ void CodeGenModule::HandleCXXStaticMemberVarInstantiation(VarDecl *VD) {
  if (DK == VarDecl::Definition && VD->hasAttr<DLLImportAttr>())
    return;

+  // Emit only those declarations that must be, when on SYCL device.
+  if (LangOpts.SYCLIsDevice && !MustBeEmitted(VD))


This doesn't seem to be the right place for this. This is the initialization, not when the variable itself is being emitted.

Can you backtrace a little to see what is causing the static member to be emitted, and do the test there?

Also, do we have the same problem with globals (non-members)? How about magic-statics?

What's happening with this is that the static variable emission (as well as globals and function-local statics) is being throttled using the usual mechanism in CodeGen and AST routines. However, the initialization is being generated via a call from Sema:HandleCXXStaticMemberVarInstantiation() and the initialization later forces the variable to be emitted as well (since it gets used).

I have moved the check earlier in Sema.

Can you point out the place where "initialization later forces the variable to be emitted as well"?

In CodeGenModule.cpp:

StringRef MangledName = getMangledName(GD); if (GetGlobalValue(MangledName) != nullptr) { // The value has already been used and should therefore be emitted. addDeferredDeclToEmit(GD);

And could you please point how exactly initialization makes variable used?

That is called from Sema: SemaTemplateInstantiateDecl.cpp via PassToConsumeRAII but there isn't enough information there to suppress it. I tried, but incorrectly; I couldn't figure out what property of Var I could use there.

Perhaps we SHOULD be marking these vardecls as 'used-by-sycl' in some manner. If they aren't referenced in a kernel, we probably shouldn't be instantiating them, right?

That way we could just suppress the instantiate-variable-definition in PerformPendingInstantiations.

Although, actually... If these are static-data-members, don't we KNOW that they can't be emitted? We have an error that prevents them from being used, right? So 'Is static member && SYCL' should be enough. Either at the beginning of InstantiateVariableDefinition or in performpendinginstantiations, though I'm not sure where would be better. See 6096 for how to check static-data-member.

That said, do Variable Templates cause us problems here too?

We diagnose non-const static data members. I will check out the other places you mentioned.

I haven't specifically tested variable templates, will do so.

Ah, right. Does this patch make sure that this gets emitted anyway if it is used then? I'd hate for us to suppress this here, but have it be used later/before.

Fznamznon · 2020-05-29T11:40:07Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+  if (getLangOpts().SYCLIsDevice && !Var->hasAttr<SYCLKernelAttr>() &&
+      !Var->hasAttr<SYCLDeviceAttr>())
+    return;


This stuff doesn't seem right. That is probably why tests are failing.
sycl_kernel attribute can be applied only to a function template.
Only SYCL_EXTERNAL functions can have sycl_device attribute.

Yes, I realize that now. Can you suggest a better approach? I could go back to my original change in CodeGenModule.cpp

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Fznamznon · 2020-06-03T09:06:45Z

Looks like this patch breaks pipes. @MrSidims , could you please take a look at the test logs? Have you seen such errors before?

MrSidims · 2020-06-03T09:13:55Z

Looks like this patch breaks pipes. @MrSidims , could you please take a look at the test logs? Have you seen such errors before?

I'll take a look, we might need to have to do some staggered changes.

MrSidims · 2020-06-03T11:36:15Z

Looks like this patch breaks pipes. @MrSidims , could you please take a look at the test logs? Have you seen such errors before?

I'll take a look, we might need to have to do some staggered changes.

According to log the failure came from FPGA emulator backend's pass, that recognizes SYCL pipes. I'm currently building the debug version of the emulator locally to reproduce, but...
...that looks weird, after applying the patch I see no changes but clang version metadata in LLVM IR module generated for fpga_pipes.cpp test, hence FPGA emulator backend flow for pipes shan't be affected. At the same time the same tests are passing on windows. Will dig more into this when build is complete.

premanandrao · 2020-06-03T12:54:28Z

Thank you @MrSidims , appreciate your help.

Signed-off-by: Premanand M Rao <[email protected]>

premanandrao · 2020-06-04T01:26:44Z

@Fznamznon and @erichkeane, if either of you could review my latest set of changes, I'd appreciate it.

erichkeane

Please stop force-pushing, it makes reviews really difficult. Second, @MrSidims , please take a look and make sure you're happy.

MrSidims · 2020-06-04T09:22:41Z

Second, @MrSidims , please take a look and make sure you're happy.

Yeah, I happy.

LGTM

premanandrao · 2020-06-04T09:49:09Z

Please stop force-pushing, it makes reviews really difficult. Second, @MrSidims , please take a look and make sure you're happy.

Sorry. I will talk offline with you and figure out the BKM when regular push fails.

Fznamznon · 2020-06-04T10:34:08Z

clang/lib/CodeGen/CodeGenModule.cpp

+      if (I.AssociatedData->getType()->getPointerAddressSpace() !=
+          VoidPtrTy->getAddressSpace())
+        ctor.add(
+            llvm::ConstantExpr::getAddrSpaceCast(I.AssociatedData, VoidPtrTy));


You are adding a new address space cast, but I don't see any address space casts in the test.

Thanks, I will modify the test to show this.

Fznamznon · 2020-06-04T10:40:36Z

clang/lib/CodeGen/CodeGenModule.cpp

+      if (I.AssociatedData->getType()->getPointerAddressSpace() !=
+          VoidPtrTy->getAddressSpace())


Should we check that it is correct to do address space cast here?
Like this check

llvm/clang/lib/CodeGen/CGExprScalar.cpp

Line 2996 in ccefc93

static void ensureSameAddrSpace(Value *&RHS, Value *&LHS,

@asavonic ?

Yes, we have to ensure that an addrspace cast is valid.

Okay, will add this check.

In the cases above, it looks like they values are both cast to the generic address space, to ensure sameness. This does not seem to be needed here, as the cast is to address space 0. However, I have changed the call to use an existing routine (which does the check as well) which is called in few other places, instead of checking it directly how I had it before.

clang/test/CodeGenSYCL/sycl-device-static-init.cpp

Signed-off-by: Premanand M Rao <[email protected]>

clang/test/CodeGenSYCL/sycl-device-static-init.cpp

erichkeane

I'm OK with this, but @Fznamznon had a pending question? Was that answer what you were looking for?

…alized Const static data members need to be either zero-initialized or constant-initialized. We were emitting all const static data members before, which needed address space casts from constant-space to private-space. This change also reverts the address-space cast introduced in intel#1774 Signed-off-by: Premanand M Rao <[email protected]>

Const static data members need to be either zero-initialized or constant-initialized. We were emitting initializers for all const static data members before, which needed invalid address space casts from constant-space to private-space. This change also reverts the address-space cast introduced in intel#1774 Signed-off-by: Premanand M Rao <[email protected]>

Const static variables need to be either zero-initialized or constant-initialized. We were emitting initializers for all const static variables before, which needed invalid address space casts from constant-space to private-space. We now diagnose when they are neither zero- nor constant-initialized and used in device code. This change also reverts the address-space cast introduced in intel#1774 Signed-off-by: Premanand M Rao <[email protected]>

This PR adds the new execution mode RegisterMapInterfaceINTEL, see the Khronos SPIRV spec here: KhronosGroup/SPIRV-Registry#176 This execution mode allows specifying a 'register' based interface for FPGA kernels. The RegisterMapInterfaceINTEL execution mode is added with a 0/1 literal based on the kernel metadata. When the metadata is: !ip_interface !N !N = !{!"csr"} The translator emits RegisterMapInterfaceINTEL 0, and when the metadata is: !ip_interface !N !N = !{!"csr", !"accept_downstream_stall"} The translator emits RegisterMapInterfaceINTEL 1 NOTE The new mode is under capability FPGAKernelAttributesv2INTEL which implicitly defines the capability FPGAKernelAttributesv2INTEL. This PR is very similar to Implement StreamingInterfaceINTEL execution mode intel#1218 Original commit: KhronosGroup/SPIRV-LLVM-Translator@a9f4f25

premanandrao requested review from erichkeane and Fznamznon as code owners May 28, 2020 16:00

premanandrao force-pushed the remote_staticinit branch from 4eeb540 to ce3a81f Compare May 28, 2020 16:04

erichkeane reviewed May 28, 2020

View reviewed changes

premanandrao force-pushed the remote_staticinit branch from ce3a81f to 9bb619d Compare May 28, 2020 21:00

erichkeane previously approved these changes May 28, 2020

View reviewed changes

Fznamznon reviewed May 29, 2020

View reviewed changes

premanandrao dismissed erichkeane’s stale review via beb531e June 2, 2020 20:02

premanandrao force-pushed the remote_staticinit branch from 9bb619d to beb531e Compare June 2, 2020 20:02

premanandrao requested review from erichkeane and Fznamznon June 2, 2020 20:25

erichkeane reviewed Jun 2, 2020

View reviewed changes

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Outdated Show resolved Hide resolved

premanandrao force-pushed the remote_staticinit branch from beb531e to ca426ec Compare June 2, 2020 21:01

erichkeane previously approved these changes Jun 2, 2020

View reviewed changes

premanandrao dismissed erichkeane’s stale review via 5a89f11 June 2, 2020 21:07

premanandrao force-pushed the remote_staticinit branch from ca426ec to 5a89f11 Compare June 2, 2020 21:07

erichkeane previously approved these changes Jun 2, 2020

View reviewed changes

premanandrao self-assigned this Jun 2, 2020

premanandrao dismissed erichkeane’s stale review via 404c964 June 3, 2020 19:11

premanandrao force-pushed the remote_staticinit branch 2 times, most recently from 404c964 to 0a562f4 Compare June 3, 2020 22:04

[SYCL] Do not emit unneeded static initializations in sycl device code

96ef141

Signed-off-by: Premanand M Rao <[email protected]>

premanandrao force-pushed the remote_staticinit branch from 0a562f4 to 96ef141 Compare June 3, 2020 22:08

erichkeane reviewed Jun 4, 2020

View reviewed changes

Fznamznon reviewed Jun 4, 2020

View reviewed changes

premanandrao added 2 commits June 4, 2020 11:15

[SYCL] Do not emit unneeded static initializations in sycl device code

016d98c

Signed-off-by: Premanand M Rao <[email protected]>

[SYCL] Do not emit unneeded static initializations in sycl device code

283a81f

Signed-off-by: Premanand M Rao <[email protected]>

Fznamznon reviewed Jun 4, 2020

View reviewed changes

clang/test/CodeGenSYCL/sycl-device-static-init.cpp Show resolved Hide resolved

bader requested review from Fznamznon, erichkeane and asavonic June 5, 2020 12:52

erichkeane approved these changes Jun 5, 2020

View reviewed changes

Fznamznon approved these changes Jun 8, 2020

View reviewed changes

bader merged commit 88d2031 into intel:sycl Jun 8, 2020

premanandrao mentioned this pull request Jul 1, 2020

[SYCL] Do not emit const static data members that are not const-… #2019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Do not emit unneeded static initializations in sycl device code #1774

[SYCL] Do not emit unneeded static initializations in sycl device code #1774

premanandrao commented May 28, 2020

erichkeane May 28, 2020

premanandrao May 28, 2020

Fznamznon May 29, 2020

premanandrao May 29, 2020

Fznamznon May 29, 2020

premanandrao May 29, 2020

erichkeane May 29, 2020

erichkeane May 29, 2020

premanandrao May 29, 2020

erichkeane May 29, 2020

Fznamznon May 29, 2020

premanandrao May 29, 2020

Fznamznon commented Jun 3, 2020

MrSidims commented Jun 3, 2020

MrSidims commented Jun 3, 2020

premanandrao commented Jun 3, 2020

premanandrao commented Jun 4, 2020

erichkeane left a comment

MrSidims commented Jun 4, 2020

premanandrao commented Jun 4, 2020

Fznamznon Jun 4, 2020

premanandrao Jun 4, 2020

Fznamznon Jun 4, 2020

asavonic Jun 4, 2020

premanandrao Jun 4, 2020

premanandrao Jun 4, 2020

erichkeane left a comment

		if (I.AssociatedData->getType()->getPointerAddressSpace() !=
		VoidPtrTy->getAddressSpace())

[SYCL] Do not emit unneeded static initializations in sycl device code #1774

[SYCL] Do not emit unneeded static initializations in sycl device code #1774

Conversation

premanandrao commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fznamznon commented Jun 3, 2020

MrSidims commented Jun 3, 2020

MrSidims commented Jun 3, 2020

premanandrao commented Jun 3, 2020

premanandrao commented Jun 4, 2020

erichkeane left a comment

Choose a reason for hiding this comment

MrSidims commented Jun 4, 2020

premanandrao commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment