Skip to content

[IR] Add TargetExtType::CanBeLocal property #99016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 22, 2024
+112 −13
Merged

[IR] Add TargetExtType::CanBeLocal property #99016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2eda0a
[IR] Add TargetExtType::CanBeAlloca property
jayfoad Jul 16, 2024
7383934
Rename to CanBeLocal. Check byval arguments.
jayfoad Aug 21, 2024
c2e94ea
Update llvm/include/llvm/IR/DerivedTypes.h
jayfoad Aug 21, 2024
99b4a5f
Merge remote-tracking branch 'origin/main' into canbealloca
jayfoad Oct 22, 2024
db1bba8
Merge remote-tracking branch 'origin/main' into canbealloca
jayfoad Nov 19, 2024
f834c16
Fix tests
jayfoad Nov 19, 2024
44fafe6
Check for nested non-CanBeLocal types
jayfoad Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/include/llvm/IR/DerivedTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,9 @@ 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 allocated on the stack, either as the allocated type
// of an alloca instruction or as a byval function parameter.
CanBeLocal = 1U << 2,
};

/// Returns true if the target extension type contains the given property.
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/IR/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,12 +826,14 @@ static TargetTypeInfo getTargetTypeInfo(const TargetExtType *Ty) {
return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::CanBeGlobal);
Copy link
Contributor Author

@jayfoad jayfoad Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nikic nikic Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jayfoad jayfoad Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jayfoad jayfoad Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jayfoad jayfoad Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (Name.starts_with("spirv."))
return TargetTypeInfo(PointerType::get(C, 0), TargetExtType::HasZeroInit,
TargetExtType::CanBeGlobal);
TargetExtType::CanBeGlobal,
TargetExtType::CanBeLocal);

// Opaque types in the AArch64 name space.
if (Name == "aarch64.svcount")
return TargetTypeInfo(ScalableVectorType::get(Type::getInt1Ty(C), 16),
TargetExtType::HasZeroInit);
TargetExtType::HasZeroInit,
TargetExtType::CanBeLocal);

return TargetTypeInfo(Type::getVoidTy(C));
}
Expand Down
12 changes: 12 additions & 0 deletions llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2021,6 +2021,12 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
SmallPtrSet<Type *, 4> Visited;
Check(Attrs.getByValType()->isSized(&Visited),
"Attribute 'byval' does not support unsized types!", V);
// Check if it's a target extension type that disallows being used on the
// stack.
if (auto *TTy = dyn_cast<TargetExtType>(Attrs.getByValType())) {
Check(TTy->hasProperty(TargetExtType::CanBeLocal),
"'byval' argument has illegal target extension type", V);
}
Check(DL.getTypeAllocSize(Attrs.getByValType()).getKnownMinValue() <
(1ULL << 32),
"huge 'byval' arguments are unsupported", V);
Expand Down Expand Up @@ -4285,6 +4291,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 on the
// stack.
if (auto *TTy = dyn_cast<TargetExtType>(AI.getAllocatedType())) {
Check(TTy->hasProperty(TargetExtType::CanBeLocal),
"Alloca has illegal target extension type", &AI);
Copy link
Contributor

@nikic nikic Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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") }.

Copy link
Contributor Author

@jayfoad jayfoad Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jayfoad jayfoad Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#116639 does this for CanBeGlobal.

Copy link
Contributor Author

@jayfoad jayfoad Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Check(AI.getArraySize()->getType()->isIntegerTy(),
"Alloca array size must have integer type", &AI);
if (MaybeAlign A = AI.getAlign()) {
Expand Down
13 changes: 13 additions & 0 deletions llvm/test/Assembler/target-type-properties.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
; 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
; RUN: not llvm-as < %t/byval.ll -o /dev/null 2>&1 | FileCheck --check-prefix=CHECK-BYVAL %s
; Check target extension type properties are verified in the assembler.

;--- zeroinit-error.ll
Expand All @@ -14,3 +16,14 @@ 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
}

;--- byval.ll
declare void @foo(ptr byval(target("spirv.Image")))
; CHECK-BYVAL: 'byval' argument has illegal target extension type