[SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE #1756

kbobrovs · 2020-05-26T05:07:33Z

REVIEW ONLY THE LAST COMMIT

"private globals" are essentially private variables in a global scope.
Marked with attribute((opencl_private)). Additionally can be
marked with attribute((register_num(n))) to bind to a specific register.

Signed-off-by: Konstantin S Bobrovsky konstantin.s.bobrovsky@intel.com

erichkeane · 2020-05-26T15:54:34Z

clang/lib/CodeGen/CodeGenModule.cpp

@@ -3872,6 +3872,13 @@ LangAS CodeGenModule::GetGlobalVarAddressSpace(const VarDecl *D) {
    if (!D || D->getType().getAddressSpace() == LangAS::Default) {
      return LangAS::opencl_global;
    }
+    if (D)


What cases exist where the VarDecl is null?

I don't know. Following the above if (!D ||...) style

this one is removed entirely

erichkeane · 2020-05-26T15:55:39Z

clang/lib/Sema/SemaDecl.cpp

@@ -12562,6 +12562,14 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
    if (getLangOpts().OpenCL &&
        Var->getType().getAddressSpace() == LangAS::opencl_local)
      return;
+    // In SYCL ESIMD device code non-constant file scope variables can't be
+    // initialized.
+    // TODO add proper diagnostics for both SYCL and OpenCL paths


This seems like a good opportunity to just diagnose these.

Are you suggesting to implement the check with this PR?
It will take time that I don't have at this point, hence TODO. This is a big feature with a number of gaps which will be addressed incrementally over time.

I think this would have to happen to unblock this PR. The feature is not complete/ready for merge until you've done your proper semantic analysis.

I will try.
Looks like OpenCL right above is not complete/ready too.

Update: this code is engaged only for implicit initialization, which shouldn't happen for private globals neither, so I just return skipping implicit initialization code creation (as OpenCL does). The check for explicit initialization is added elsewhere in SemaDecl.cpp, plus a test.

erichkeane · 2020-05-26T15:56:22Z

clang/lib/Sema/SemaDeclAttr.cpp

+        << AL << 0;
+    return;
+  }
+  assert(AL.getNumArgs() == 1);


Do you have a test to make sure that this assert isn't hit when providing more args? A number of functions here check to make sure they have the right number of args.

I don't have a test. Is there a policy that each assert should have a test?

Well, you're doing something significantly different from what every other diagnostic happens. I presumed you thought the assert was safe (rather than checking that you have the correct number of args like everyone else here), so I wanted you to confirm it with a test.

OK, I see your point now. I will change the assert to compiler check with diagnostics and add a negative test.

Fznamznon · 2020-05-26T19:41:08Z

clang/include/clang/Basic/Attr.td

+// Marks functions which must not be vectorized via horizontal SIMT widening,
+// e.g. because the function is already vectorized. Used to mark SYCL
+// explicit SIMD kernels and functions.
+def SYCLSimd : InheritableAttr {


Wow, I have seen this attribute in the previous patch.
Should It really be here?

right, it should not.

Fznamznon · 2020-05-26T19:46:22Z

clang/include/clang/Basic/Attr.td

+def SYCLSimd : InheritableAttr {
+  let Spellings = [GNU<"sycl_explicit_simd">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [SYCLIsDevice];


But I just realized, should LangOpts value be SYCLExplicitSIMD instead of SYCLIsDevice?

AlexeySachkov · 2020-05-27T13:00:36Z

clang/lib/CodeGen/CodeGenModule.cpp

+    if (AddrSpace == LangAS::opencl_private ||
+        AddrSpace == LangAS::opencl_local)
+      // SYCL explicit SIMD path: recognize globals in private or local address
+      // space.
+      return AddrSpace;


You don't have a check in codegen test that these address spaces are actually preserved and not replaced with something else by return getTargetCodeGenInfo().getGlobalVarAddressSpace(*this, D);

why would I need that test?

Apparently, you need this particular change here which changes an address space of some variables. However, I don't see any tests that check for particular address space

@DenisBakhvalov agreed to help here with the test

Apparently, we found that this check is not needed anymore. Removed.

kbobrovs · 2020-05-29T19:24:05Z

@erichkeane, @Fznamznon, @AlexeySachkov - ping

Fznamznon

I don't have objections, but I'd like @AlexeySachkov to approve first.

erichkeane

Did my run-through, but @AlexeySachkov and @Fznamznon should both look at this and be familiar with it as well.

erichkeane · 2020-05-29T19:33:32Z

clang/test/SemaSYCL/esimd-private-global.cpp

+// expected-error@+1{{'register_num' attribute takes one argument}}
+__attribute__((opencl_private)) __attribute__((register_num(10, 11))) int privGlob2;
+
+// expected-error@+1{{(SYCL explicit SIMD) private global variable cannot have an initializer}}


Why is SYCL explicit SIMD in parens in this message? It likely should be just SYCL Explicit SIMD private global....

This designates the special mode. I expect there will be other messages tagged similarly, where absence of braces won't make sense.

Sorry, I don't get it? We don't have anything like that elsewhere in the compiler, so this would be a completely new designation.

My point is that we'll likely have more of these. This is not adjective to "private global" but the "explicit SIMD" mode tag.

SYCL explicit SIMD does not permit private global variable to have an initializer

We discussed a reword of this error message on the call, it seems it hasn't happened yet.

Right, overlooked this one

…n Front End. "private globals" are essentially private variables in a global scope. Marked with __attribute__((opencl_private)). Additionally can be marked with __attribute__((register_num(n))) to bind to a specific register. Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

- Better checking and diagnostics - More tests Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs · 2020-06-08T18:54:35Z

@AlexeySachkov, please take a look/approve.

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

* upstream/sycl: [SYCL] Fix broken build after introduction USM to ocl headers (intel#1852) [SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE (intel#1756) [SYCL] Introduce interop handle for host task (intel#1747)

kbobrovs requested review from AGindinson, erichkeane, Fznamznon and mdtoguchi as code owners May 26, 2020 05:07

kbobrovs force-pushed the esimd-glob branch from 33eedea to b2d4488 Compare May 26, 2020 05:17

erichkeane reviewed May 26, 2020

View reviewed changes

Fznamznon reviewed May 26, 2020

View reviewed changes

AlexeySachkov reviewed May 27, 2020

View reviewed changes

kbobrovs force-pushed the esimd-glob branch from 6b5bf40 to 7bbc51d Compare May 27, 2020 23:20

kbobrovs requested review from erichkeane, Fznamznon and AlexeySachkov May 27, 2020 23:51

Fznamznon reviewed May 29, 2020

View reviewed changes

erichkeane reviewed May 29, 2020

View reviewed changes

kbobrovs requested a review from erichkeane May 29, 2020 21:17

kbobrovs added 2 commits June 5, 2020 19:32

[SQUASH] Addressed review comments.

43f3f1e

- Better checking and diagnostics - More tests Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs force-pushed the esimd-glob branch from 8e8ed41 to 43f3f1e Compare June 6, 2020 03:39

[SQUASH] Review: disabled pragma syntax for register_num attribute

d616223

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

[SQUASH] Review: changed error message for initializer check.

21ec553

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs requested review from elizabethandrews and premanandrao as code owners June 9, 2020 01:11

AlexeySachkov approved these changes Jun 9, 2020

View reviewed changes

erichkeane approved these changes Jun 9, 2020

View reviewed changes

intel deleted a comment from kbobrovs Jun 9, 2020

pvchupin merged commit bba5911 into intel:sycl Jun 9, 2020

kbobrovs deleted the esimd-glob branch July 30, 2020 12:30

Fznamznon mentioned this pull request Jan 19, 2021

[NFC][SYCL] Rename poorly named diagnostic #3056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE #1756

[SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE #1756

kbobrovs commented May 26, 2020

erichkeane May 26, 2020

kbobrovs May 27, 2020

kbobrovs Jun 8, 2020

erichkeane May 26, 2020

kbobrovs May 27, 2020

erichkeane May 27, 2020

kbobrovs May 27, 2020

kbobrovs May 27, 2020

erichkeane May 26, 2020

kbobrovs May 27, 2020

erichkeane May 27, 2020

kbobrovs May 27, 2020

Fznamznon May 26, 2020

kbobrovs May 27, 2020

Fznamznon May 26, 2020 •

edited

Loading

kbobrovs May 27, 2020

AlexeySachkov May 27, 2020

kbobrovs May 27, 2020

AlexeySachkov Jun 2, 2020

kbobrovs Jun 4, 2020

DenisBakhvalov Jun 5, 2020

kbobrovs commented May 29, 2020

Fznamznon left a comment

erichkeane left a comment

erichkeane May 29, 2020

kbobrovs May 29, 2020 •

edited

Loading

erichkeane May 29, 2020

kbobrovs May 29, 2020

kbobrovs Jun 2, 2020

erichkeane Jun 8, 2020

kbobrovs Jun 9, 2020

kbobrovs commented Jun 8, 2020

[SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE #1756

[SYCL][ESIMD] Implement "private globals" (file scope private vars) in FE #1756

Conversation

kbobrovs commented May 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fznamznon May 26, 2020 • edited Loading

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

kbobrovs commented May 29, 2020

Fznamznon left a comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbobrovs May 29, 2020 • edited Loading

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

kbobrovs commented Jun 8, 2020

Fznamznon May 26, 2020 •

edited

Loading

kbobrovs May 29, 2020 •

edited

Loading