[SYCL] Emit an aliased function only if it is used #2430

premanandrao · 2020-09-04T17:58:09Z

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

Fznamznon

Am I right that alias attribute somehow forces clang to emit functions even they are unused in device code?

Fznamznon · 2020-09-07T12:52:43Z

clang/lib/CodeGen/CodeGenModule.cpp

+      auto DDI = DeferredDecls.find(AliaseeName);
+      // Emit what is aliased first.
+      if (DDI != DeferredDecls.end()) {
+        llvm::GlobalValue *AliaseeGV = dyn_cast<llvm::GlobalValue>(


It is possible that dyn_cast will return nullptr, please use cast here if only GlobalValue type is asumed. Add check on nullptr and exit otherwise.

Thanks for this. Done.

Fznamznon · 2020-09-07T12:58:31Z

clang/lib/CodeGen/CodeGenModule.cpp

+      for (auto it = DeferredAliases.begin(); it != DeferredAliases.end();
+           ++it) {


Suggested change

for (auto it = DeferredAliases.begin(); it != DeferredAliases.end();

++it) {

for (auto It : DeferredAliases) {

I can't do range for here since I am invalidating the iterator within the body. Which made me realize I had a bug in my previous for loop, which I have fixed.

Fznamznon · 2020-09-07T12:59:23Z

clang/lib/CodeGen/CodeGenModule.cpp

-    return EmitAliasDefinition(GD);
+  // handle it now.
+  if (Global->hasAttr<AliasAttr>()) {
+    // Emit the alias here if not for SYCL device.


How about

Suggested change

// Emit the alias here if not for SYCL device.

// Emit the alias here if it is not SYCL device compilation.

Fznamznon · 2020-09-07T14:37:43Z

clang/lib/CodeGen/CodeGenModule.cpp

+    // is used.
+    StringRef MangledName = getMangledName(GD);
+    DeferredDecls[MangledName] = GD;
+    StringRef AliaseeName = Global->getAttr<AliasAttr>()->getAliasee();


Suggest rewrite if on line 2654 and this line with

if (AliasAttr *Attr = Global->getAttr<AliasAttr>()) { ... StringRef AliaseeName = Attr->getAliasee();

Fznamznon · 2020-09-07T14:39:33Z

clang/test/CodeGenSYCL/sycl-device-alias.cpp

+// Test that aliasing does not force an unused function to be emitted
+
+// CHECK-NOT: define spir_func void @unused_func()
+extern "C" void unused_func() {}


Seems like alias attribute can be also applied to a global variable. Could you please add corresponding tests?

Thanks, I did this. (I didn't change the title to avoid needing to force-push, but I will do that when there are no further code-review comments.)

I think test renaming shouldn't require doing force-push. See #2420 for example. It has a commit which just renames a test - 8b24581 . I didn't do any force-pushes there. Also, current title works for me as well.

Fznamznon · 2020-09-07T14:42:42Z

clang/lib/CodeGen/CodeGenModule.cpp

+      auto DDI = DeferredDecls.find(AliaseeName);
+      // Emit what is aliased first.
+      if (DDI != DeferredDecls.end()) {
+        llvm::GlobalValue *AliaseeGV = dyn_cast<llvm::GlobalValue>(


BTW, is llvm namespace required here?

Looks like it is.

Fznamznon · 2020-09-07T14:44:58Z

clang/lib/CodeGen/CodeGenModule.cpp

+    if (LangOpts.SYCLIsDevice && VD->hasAttr<AliasAttr>()) {
+      StringRef AliaseeName = VD->getAttr<AliasAttr>()->getAliasee();


You can use one call of getAttr instead of doing hasAttr & getAttr both here as well.

Fznamznon

Looks ok except some code-style nits

Fznamznon · 2020-09-11T08:53:43Z

clang/lib/CodeGen/CodeGenModule.cpp

+    if (LangOpts.SYCLIsDevice)
+      if (AliasAttr *Attr = VD->getAttr<AliasAttr>()) {
+        StringRef AliaseeName = Attr->getAliasee();
+        auto DDI = DeferredDecls.find(AliaseeName);
+        // Emit what is aliased first.
+        if (DDI != DeferredDecls.end()) {
+          llvm::GlobalValue *AliaseeGV = dyn_cast<llvm::GlobalValue>(
+              GetAddrOfGlobal(DDI->second, ForDefinition));
+          if (!AliaseeGV)
+            AliaseeGV = GetGlobalValue(getMangledName(DDI->second));
+          assert(AliaseeGV);
+          EmitGlobalDefinition(DDI->second, AliaseeGV);
+          // Remove the entry just added to the DeferredDeclsToEmit
+          // since we have emitted it.
+          DeferredDeclsToEmit.pop_back();
+        }
+        // Now emit the alias itself.
+        EmitAliasDefinition(D);
+        continue;
+      }


Since if on line 2271 is braced, could you please add braces to if on line 2270 as well?

Suggested change

if (LangOpts.SYCLIsDevice)

if (AliasAttr *Attr = VD->getAttr<AliasAttr>()) {

StringRef AliaseeName = Attr->getAliasee();

auto DDI = DeferredDecls.find(AliaseeName);

// Emit what is aliased first.

if (DDI != DeferredDecls.end()) {

llvm::GlobalValue *AliaseeGV = dyn_cast<llvm::GlobalValue>(

GetAddrOfGlobal(DDI->second, ForDefinition));

if (!AliaseeGV)

AliaseeGV = GetGlobalValue(getMangledName(DDI->second));

assert(AliaseeGV);

EmitGlobalDefinition(DDI->second, AliaseeGV);

// Remove the entry just added to the DeferredDeclsToEmit

// since we have emitted it.

DeferredDeclsToEmit.pop_back();

}

// Now emit the alias itself.

EmitAliasDefinition(D);

continue;

}

if (LangOpts.SYCLIsDevice) {

if (AliasAttr *Attr = VD->getAttr<AliasAttr>()) {

StringRef AliaseeName = Attr->getAliasee();

auto DDI = DeferredDecls.find(AliaseeName);

// Emit what is aliased first.

if (DDI != DeferredDecls.end()) {

llvm::GlobalValue *AliaseeGV = dyn_cast<llvm::GlobalValue>(

GetAddrOfGlobal(DDI->second, ForDefinition));

if (!AliaseeGV)

AliaseeGV = GetGlobalValue(getMangledName(DDI->second));

assert(AliaseeGV);

EmitGlobalDefinition(DDI->second, AliaseeGV);

// Remove the entry just added to the DeferredDeclsToEmit

// since we have emitted it.

DeferredDeclsToEmit.pop_back();

}

// Now emit the alias itself.

EmitAliasDefinition(D);

continue;

}

}

Fznamznon · 2020-09-11T08:54:42Z

clang/lib/CodeGen/CodeGenModule.cpp

+        if (It->first == getMangledName(D)) {
+          EmitAliasDefinition(Global);
+          It = DeferredAliases.erase(It);
+        } else
+          ++It;


Could you please add braces to the else branch to make this if uniform?

Suggested change

if (It->first == getMangledName(D)) {

EmitAliasDefinition(Global);

It = DeferredAliases.erase(It);

} else

++It;

if (It->first == getMangledName(D)) {

EmitAliasDefinition(Global);

It = DeferredAliases.erase(It);

} else {

++It;

}

Fznamznon · 2020-09-11T08:57:45Z

clang/test/CodeGenSYCL/sycl-device-alias.cpp

+// Test that aliasing does not force an unused function to be emitted
+
+// CHECK-NOT: define spir_func void @unused_func()
+extern "C" void unused_func() {}


I think test renaming shouldn't require doing force-push. See #2420 for example. It has a commit which just renames a test - 8b24581 . I didn't do any force-pushes there. Also, current title works for me as well.

premanandrao · 2020-09-11T15:28:34Z

I meant updating the title of the commit itself. Right now it says: "Emit an aliased function ...", I want to change it to "Emit an aliased entity ..." instead. I think I need to do a 'git commit --amend' for that, and that makes me do a subsequent force-push.

Signed-off-by: Premanand M Rao <[email protected]> Fix clang-format error Address code-review comments clang-format changes Address review comments about code style

Fznamznon · 2020-09-11T15:49:30Z

I meant updating the title of the commit itself. Right now it says: "Emit an aliased function ...", I want to change it to "Emit an aliased entity ..." instead. I think I need to do a 'git commit --amend' for that, and that makes me do a subsequent force-push.

I think you can change the title of PR then. Anyway someone who will merge this PR will squash your commits, he will likely use title and description of the whole PR, not the specific commit.

Fznamznon · 2020-09-11T15:53:39Z

clang/test/CodeGenSYCL/sycl-device-alias.cpp

+
+// CHECK-DAG: @used_int = addrspace(1) constant i32 5, align 4
+extern "C" const int used_int = 5;
+// CHECK-DAG: @alias_used_int = alias i32, addrspacecast (i32 addrspace(1)* @used_int to i32*)


Wow, sorry, but I've just notified that we have incorrect address space cast here. It casts from global (1) address space to private (no number or 0), this is not correct. We can cast only non-constant address space (1,3,0 in spir targets) to generic address space (4 in spir target).

Thanks for catching this; this isn't due to my change, but I can change that if it is broken. What needs to happen here?

Also, is 0 private or just the default? I thought private was 4.

It looks like 'used_int' is allocated in addrspace 1; and when the alias is created, it doesn't consider the addrspace of the aliasee (but rather the address space of its type). I will look into changing that.

Also, is 0 private or just the default? I thought private was 4.

There are difference between AST address spaces and IR address spaces. So, OpenCL/SYCL has global, constant, local, private and generic address spaces. They are mapped to some numbers in IR. These numbers actually depend on target. We mostly use SPIR-based target. So, in SPIR targets 1 is global, 2 is constant, 3 is local, 4 is generic, 0 is private. In AST we also have address space qualifiers, and in our SYCL implementation if pointer has some AS qualifier, it will have this same qualifier in AST. I.e. if it was expliticly declared, i.e. __global int *ptr etc, this pointer will have global AS qualifier in AST and it will be codegen-ed with address space "1". But, if some pointer doesn't have address space qualifier it will have some default address space in AST. Here is the thing that two pointers with different address spaces but with the same type are considered as pointers of different type (i.e. __global int* and __local int* are different types), but, In SYCL user doesn't have to define different overload for each function but with different address spaces in parameters, so the conversion should be done by the compiler. So, our solution was to map default AST address space to generic address space in SPIR target, because it is valid to cast from most of address spaces to generic. So, for each place where default address space was in AST, we emit generic (4 in SPIR target), so the following tools and backends read it as generic. If I remember correct the only thing that we left in SPIR private address space is device-side allocas, because it is not possible to allocate with generic address space.

@Fznamznon, please take a look. I think I have addressed this issue.

Fznamznon

Thanks!
p.s. sycl-intelfpga-static-lib.cpp LIT is known failure.

* upstream/sycl: (405 commits) [SYCL] Implement new env var SYCL_DEVICE_FILTER (intel#2239) [Driver][SYCL] Make /MD the default for -fsycl (intel#2478) [SYCL]: basic support of contexts with multiple devices in Level-Zero (intel#2440) [SYCL] Fix LIT regression after 9dd18ca (intel#2481) [SYCL][L0] Kernel Destroy in piKernelRelease (intel#2475) [SYCL] Emit an aliased function only if it is used (intel#2430) [Driver][SYCL] Add defaultlib directive for sycl lib (intel#2464) [Driver][SYCL] Improve situations where .exe is added for AOT tools (intel#2467) [SYCL][L0]: Check Queue refcnt prior to using members in event wait/release (intel#2471) [SYCL] Unroll several loops in __init method accessor class (intel#2449) [SYCL][Doc] Add link to use pinned memory spec (intel#2463) [SYCL] Link SYCL device libraries by default. (intel#2400) Revert "[SYCL] XFAIL test blcoking pulldown" Avoid usage of deprecated "VectorType::getNumElements" (intel#737) Fix nullptr dereference (intel#741) Do not translate arbitrary precision operations without corresponding extensions (intel#714) Add Constrained Floating-Point Intrinsics support [SYCL] Take into account auxiliary cmake options for Level Zero loader [InstCombine] improve fold of pointer differences [InstCombine] add ptr difference tests; NFC ...

This fixes a bug introduced in PR intel#2430 where the iterator might get invalidated between accesses. Signed-off-by: Premanand M Rao <[email protected]>

This fixes a bug introduced in PR #2430 where the iterator might get invalidated between accesses. Signed-off-by: Premanand M Rao <[email protected]>

[UR] Fix correct usage of In Order sync list given counting events

premanandrao requested review from elizabethandrews and Fznamznon as code owners September 4, 2020 17:58

Fznamznon reviewed Sep 7, 2020

View reviewed changes

Fznamznon reviewed Sep 11, 2020

View reviewed changes

[SYCL] Emit an aliased entity only if it is used

f5dd0c0

Signed-off-by: Premanand M Rao <[email protected]> Fix clang-format error Address code-review comments clang-format changes Address review comments about code style

premanandrao force-pushed the remote_alias branch from 8c683e5 to f5dd0c0 Compare September 11, 2020 15:44

Fznamznon reviewed Sep 11, 2020

View reviewed changes

premanandrao added 4 commits September 11, 2020 12:51

Testing

eb648e0

Fix address-space issues

d91164f

Fix clang-format issue

1671056

Remove extraneous comment

62df4ea

Fznamznon approved these changes Sep 16, 2020

View reviewed changes

romanovvlad merged commit 5a2fe9c into intel:sycl Sep 16, 2020

premanandrao mentioned this pull request Sep 18, 2020

[SYCL] Use a copy to avoid invalidated access #2495

Merged

romanovvlad pushed a commit that referenced this pull request Sep 21, 2020

[SYCL] Use a copy to avoid invalidated access (#2495)

6832d83

This fixes a bug introduced in PR #2430 where the iterator might get invalidated between accesses. Signed-off-by: Premanand M Rao <[email protected]>

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2430 from nrspruit/fix_inorder_initList

ce69da1

[UR] Fix correct usage of In Order sync list given counting events

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Emit an aliased function only if it is used #2430

[SYCL] Emit an aliased function only if it is used #2430

premanandrao commented Sep 4, 2020

Fznamznon left a comment

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 11, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon Sep 7, 2020

premanandrao Sep 10, 2020

Fznamznon left a comment

Fznamznon Sep 11, 2020

premanandrao Sep 11, 2020

Fznamznon Sep 11, 2020

premanandrao Sep 11, 2020

Fznamznon Sep 11, 2020

premanandrao commented Sep 11, 2020

Fznamznon commented Sep 11, 2020

Fznamznon Sep 11, 2020

premanandrao Sep 11, 2020

premanandrao Sep 11, 2020

Fznamznon Sep 11, 2020

premanandrao Sep 15, 2020

Fznamznon left a comment •

edited

Loading

		for (auto it = DeferredAliases.begin(); it != DeferredAliases.end();
		++it) {

	for (auto it = DeferredAliases.begin(); it != DeferredAliases.end();
	++it) {
	for (auto It : DeferredAliases) {

	// Emit the alias here if not for SYCL device.
	// Emit the alias here if it is not SYCL device compilation.

		if (LangOpts.SYCLIsDevice && VD->hasAttr<AliasAttr>()) {
		StringRef AliaseeName = VD->getAttr<AliasAttr>()->getAliasee();

[SYCL] Emit an aliased function only if it is used #2430

[SYCL] Emit an aliased function only if it is used #2430

Conversation

premanandrao commented Sep 4, 2020

Fznamznon left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fznamznon left a 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

premanandrao commented Sep 11, 2020

Fznamznon commented Sep 11, 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

Fznamznon left a comment • edited Loading

Choose a reason for hiding this comment

Fznamznon left a comment •

edited

Loading