[IR] Add TargetExtType::CanBeLocal property #99016

jayfoad · 2024-07-16T10:34:22Z

Add a property to allow marking target extension types that cannot be
used in an alloca instruction or byval argument, similar to CanBeGlobal
for global variables.

llvmbot · 2024-07-16T10:34:52Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-ir

Author: Jay Foad (jayfoad)

Changes

Add a property to allow marking target extension types that cannot be
used in an alloca instruction, similar to CanBeGlobal for global
variables.

Full diff: https://github.com/llvm/llvm-project/pull/99016.diff

4 Files Affected:

(modified) llvm/include/llvm/IR/DerivedTypes.h (+2)
(modified) llvm/lib/IR/Type.cpp (+4-2)
(modified) llvm/lib/IR/Verifier.cpp (+6)
(modified) llvm/test/Assembler/target-type-properties.ll (+8)

diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index 01f76d4932780..054cb370bc316 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -769,6 +769,8 @@ class TargetExtType : public Type {
     HasZeroInit = 1U << 0,
     /// This type may be used as the value type of a global variable.
     CanBeGlobal = 1U << 1,
+    /// This type may be used as the allocated type of an alloca instruction.
+    CanBeAlloca = 1U << 2,
   };
 
   /// Returns true if the target extension type contains the given property.
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index 5c61ad9f000b0..3f982ef520e92 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -838,12 +838,14 @@ static TargetTypeInfo getTargetTypeInfo(const TargetExtType *Ty) {
     return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::CanBeGlobal);
   if (Name.starts_with("spirv."))
     return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::HasZeroInit,
-                          TargetExtType::CanBeGlobal);
+                          TargetExtType::CanBeGlobal,
+                          TargetExtType::CanBeAlloca);
 
   // Opaque types in the AArch64 name space.
   if (Name == "aarch64.svcount")
     return TargetTypeInfo(ScalableVectorType::get(Type::getInt1Ty(C), 16),
-                          TargetExtType::HasZeroInit);
+                          TargetExtType::HasZeroInit,
+                          TargetExtType::CanBeAlloca);
 
   return TargetTypeInfo(Type::getVoidTy(C));
 }
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 75a53c1c99734..e59dc6bf24d38 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4273,6 +4273,12 @@ void Verifier::visitAllocaInst(AllocaInst &AI) {
   SmallPtrSet<Type*, 4> Visited;
   Check(AI.getAllocatedType()->isSized(&Visited),
         "Cannot allocate unsized type", &AI);
+  // Check if it's a target extension type that disallows being used in an
+  // alloca.
+  if (auto *TTy = dyn_cast<TargetExtType>(AI.getAllocatedType())) {
+    Check(TTy->hasProperty(TargetExtType::CanBeAlloca),
+          "Alloca has illegal target extension type", &AI);
+  }
   Check(AI.getArraySize()->getType()->isIntegerTy(),
         "Alloca array size must have integer type", &AI);
   if (MaybeAlign A = AI.getAlign()) {
diff --git a/llvm/test/Assembler/target-type-properties.ll b/llvm/test/Assembler/target-type-properties.ll
index 49c9d812f1cf4..eae5eec04da85 100644
--- a/llvm/test/Assembler/target-type-properties.ll
+++ b/llvm/test/Assembler/target-type-properties.ll
@@ -1,6 +1,7 @@
 ; RUN: split-file %s %t
 ; RUN: not llvm-as < %t/zeroinit-error.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-ZEROINIT %s
 ; RUN: not llvm-as < %t/global-var.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-GLOBALVAR %s
+; RUN: not llvm-as < %t/alloca.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-ALLOCA %s
 ; Check target extension type properties are verified in the assembler.
 
 ;--- zeroinit-error.ll
@@ -14,3 +15,10 @@ define void @foo() {
 ;--- global-var.ll
 @global = external global target("unknown_target_type")
 ; CHECK-GLOBALVAR: Global @global has illegal target extension type
+
+;--- alloca.ll
+define void @foo() {
+  %val = alloca target("spirv.Image")
+; CHECK-ALLOCA: Alloca has illegal target extension type
+  ret void
+}

jayfoad · 2024-07-16T10:39:23Z

llvm/lib/IR/Type.cpp

@@ -838,12 +838,14 @@ static TargetTypeInfo getTargetTypeInfo(const TargetExtType *Ty) {
    return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::CanBeGlobal);


I made a complete guess here that spirv.Image should not be used in allocas, so that I could use it as a test case. Please let me know if I got that wrong.

Maybe @jcranmer-intel can comment on which spirv. types can/cannot be used in allocas.

There are now some tests which do use spirv.Image in allocas:

test/CodeGen/SPIRV/transcoding/image_with_access_qualifiers.ll test/CodeGen/SPIRV/transcoding/get_image_num_mip_levels.ll test/CodeGen/SPIRV/image-unoptimized.ll test/CodeGen/SPIRV/read_image.ll

@jcranmer-intel any comments on the validity of this? Is it a supported use of spirv.Image, or just something that was conventient for these test cases?

Because of this I don't have any non-CanBeLocal types to test with, so the tests I added in type/Assembler/target-type-properties.ll are currently failing.

I'm now using the newly added amdgcn.named.barrier for testing.

nikic

The use case here is specifically a type that is sized but cannot be used in alloca?

What about use in byval?

jayfoad · 2024-07-17T10:01:26Z

The use case here is specifically a type that is sized but cannot be used in alloca?

Yes. The use case I am interested in is special global objects that correspond to hardware resources that will be statically allocated by the backend. They can never be dynamically allocated.

What about use in byval?

We definitely don't want them in byval - thanks!

Perhaps MustBeGlobal would be a better name to describe this property?

nhaehnle · 2024-08-13T18:36:06Z

What's the status of this? It seems reasonable to me. In terms of the name, I would slightly prefer something that relaxes restrictions instead of adding them, so perhaps CanBeStack (to cover both byval and alloca)?

Add a property to allow marking target extension types that cannot be used in an alloca instruction, similar to CanBeGlobal for global variables.

jayfoad · 2024-08-21T09:37:15Z

so perhaps CanBeStack (to cover both byval and alloca)?

I went for CanBeLocal but I'm happy to change it again if reviewers want.

llvm/include/llvm/IR/DerivedTypes.h

nikic · 2024-08-21T10:48:06Z

llvm/lib/IR/Verifier.cpp

+  // stack.
+  if (auto *TTy = dyn_cast<TargetExtType>(AI.getAllocatedType())) {
+    Check(TTy->hasProperty(TargetExtType::CanBeLocal),
+          "Alloca has illegal target extension type", &AI);


Also applies to the existing CanBeGlobal check, but this probably needs to be recursive? We'd not want to forbid target("foo") but then allow { target("foo") }.

Yeah, I asked about that here: https://discourse.llvm.org/t/rfc-adding-opaque-types-to-llvm-ir/65326/25

I might work on it but I want to keep that separate.

#116639 does this for CanBeGlobal.

I've now added the recursive checks to this patch too.

Co-authored-by: Nikita Popov <[email protected]>

jayfoad · 2024-11-21T12:01:34Z

Ping. This is ready again for review.

jayfoad · 2024-11-21T12:03:30Z

llvm/include/llvm/IR/DerivedTypes.h

+    SCDB_ContainsNonLocalTargetExtType = 64,
+    SCDB_NotContainsNonLocalTargetExtType = 128,


TBH I'm not sure if it's really worth adding these flags, just for an IR verifier check. I guess it accelerates recursive checking of pathological struct types with lots of repeated element types.

llvmbot added the llvm:ir label Jul 16, 2024

jayfoad requested review from nikic, paulwalker-arm, jcranmer-intel, michalpaszkowski, sdesmalen-arm and yubingex007-a11y July 16, 2024 10:36

jayfoad commented Jul 16, 2024

View reviewed changes

jayfoad requested a review from nhaehnle July 16, 2024 10:40

nikic reviewed Jul 16, 2024

View reviewed changes

jayfoad added 2 commits August 21, 2024 10:10

[IR] Add TargetExtType::CanBeAlloca property

c2eda0a

Add a property to allow marking target extension types that cannot be used in an alloca instruction, similar to CanBeGlobal for global variables.

Rename to CanBeLocal. Check byval arguments.

7383934

jayfoad force-pushed the canbealloca branch from 4aeea63 to 7383934 Compare August 21, 2024 09:35

jayfoad changed the title ~~[IR] Add TargetExtType::CanBeAlloca property~~ [IR] Add TargetExtType::CanBeLocal property Aug 21, 2024

nikic reviewed Aug 21, 2024

View reviewed changes

jayfoad and others added 4 commits August 21, 2024 11:52

Update llvm/include/llvm/IR/DerivedTypes.h

c2e94ea

Co-authored-by: Nikita Popov <[email protected]>

Merge remote-tracking branch 'origin/main' into canbealloca

99b4a5f

Merge remote-tracking branch 'origin/main' into canbealloca

db1bba8

Fix tests

f834c16

llvmbot added clang Clang issues not falling into any other category backend:AMDGPU labels Nov 19, 2024

Check for nested non-CanBeLocal types

44fafe6

jayfoad commented Nov 21, 2024

View reviewed changes

jayfoad requested a review from cmc-rep November 21, 2024 12:04

cmc-rep approved these changes Nov 21, 2024

View reviewed changes

jayfoad merged commit 294c5cb into llvm:main Nov 22, 2024
6 of 8 checks passed

jayfoad deleted the canbealloca branch November 22, 2024 10:02

		@@ -838,12 +838,14 @@ static TargetTypeInfo getTargetTypeInfo(const TargetExtType *Ty) {
		return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::CanBeGlobal);

		SCDB_ContainsNonLocalTargetExtType = 64,
		SCDB_NotContainsNonLocalTargetExtType = 128,

[IR] Add TargetExtType::CanBeLocal property #99016

[IR] Add TargetExtType::CanBeLocal property #99016

Uh oh!

Conversation

jayfoad commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

jayfoad commented Jul 17, 2024

Uh oh!

nhaehnle commented Aug 13, 2024

Uh oh!

jayfoad commented Aug 21, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayfoad commented Nov 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayfoad commented Jul 16, 2024 •

edited

Loading

llvmbot commented Jul 16, 2024 •

edited

Loading