This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Add LLVM TargetExtType
ClosedPublic

Authored by LukasSommerTu on May 25 2023, 7:09 AM.

Details

Summary

Add support for the llvm::TargetExtType to the MLIR LLVM dialect.

Target extension types were introduced to represent target-specific types, which are opaque to the compiler and optimizations.

The patch also enforces some of the constraints defined for the target extension type in the LLVM language reference manual.

Signed-off-by: Lukas Sommer <lukas.sommer@codeplay.com>

Diff Detail

Event Timeline

LukasSommerTu created this revision.May 25 2023, 7:09 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
LukasSommerTu requested review of this revision.May 25 2023, 7:09 AM
gysit added a comment.May 25 2023, 8:32 AM

Thanks for the revision.

I added a first set of comments. I guess it could be nice to have an explicit zero intializer operation for this use case instead of using an integer. Luckily there seem to be others that are also interested in such an operation. So maybe that is an opportunity to get this going?

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
270–272

Shouldn't this just be:

if (failed(verifyOpaquePtr(getOperation(), ptrType, getElemType()))
  return failure();

verifyOpaque pointer seems to return a LogicalResult?

745–746

supportsAlloca seems unrelated to atomic operations?

1850

nit: You can drop braces for this if statement since the body is trivial, i.e. a single statement.

Please also drop braces in quite a few places below.

1852

nit: is allowed for a global value with

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
145

Any chance this could be split in a function that parses types until it finds type and a second function that parses integer until there are no more parameters. I think that could simplify the logic somewhat.

Also are you sure there is no helper to parse comma separated lists?

mlir/lib/Target/LLVMIR/ModuleImport.cpp
1077–1080

I guess a cleaner way to model this would be the introduction of a zero initializer operation as discussed here:

https://discourse.llvm.org/t/zero-initialization-for-globals-with-common-linkage/1762

It may be worth synchronizing with the original author of the thread?

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
346

nit: Please host the casting out of the condition.

mlir/test/Target/LLVMIR/Import/target-ext-type.ll
13

I think it could be nice to have import export tests for global and constant operations as well?

Thanks for working on this. I added some nit comments and suggestions on how to improve minor parts.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
271

NIT: One usually writes this as

if(failed(verifyFunc()))
  return failure();
mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
116

NIT: Even though the other functions in this file don't have it, comments should be added to these newly added functions.

123

Maybe it would be easier to clearly separate types and integers into two groups to simplify this logic.

144

NIT: Can you add this grammar rule to the function comment?
Furthermore, can you explicitly add parentheses to make it clear to what part the | separates?

145

NIT: you can use AsmParser::parseCommaSeparatedList to simplify this.

LukasSommerTu marked 8 inline comments as done.

Simplify parsing logic and use existing helper

Extend and add tests for import/export

LukasSommerTu marked an inline comment as done and an inline comment as not done.May 26 2023, 4:38 AM

@Dinistro and @gysit Thanks for your fast reviews! Addressed your feedback in an updated revision and some comments inline.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

This function is used during verification of StoreOp and LoadOp, to which similar constraints regarding target extension types apply as to alloca.

Adding load and store to supportsAlloca seemed to make the name unnecessarily long, so I used this shorter name. If you prefer, I can rename it to be clearer.

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
145

parsePrettyLLVMType has no optional version, so calling it first would print an error if a target extension type only has integer parameters.

As suggested by @Dinistro, I used the existing AsmParser::parseCommaSeparatedList helper to simplify the parsing logic.

mlir/lib/Target/LLVMIR/ModuleImport.cpp
1077–1080

Yes, I agree that a LLVM dialect operation for zeroinitializer would be useful for target extension types.

I came across that discussion when initially trying to find a way to represent the zeroinitializer for target extension types. I put a comment on the discussion in Discourse.

mlir/test/Target/LLVMIR/Import/target-ext-type.ll
13

Added and extended tests to also check import/export of globals and constants.

LukasSommerTu marked an inline comment as done.May 26 2023, 4:39 AM
gysit added a comment.May 28 2023, 4:24 AM

Thanks for reviving the discussion upstream. Let's see if it gains traction after the holiday. I may also be ok to put TODOs in this revision and only adopt the zeropinitializer operation in a follow up revision.

I have some more questions regarding the load store operation verification and some nit comments. Otherwise, things are looking good.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

You are right this is indeed also called for LoadOp / StoreOp if it is an atomic load or store:

if (memOp.getOrdering() != AtomicOrdering::not_atomic) {
  if (!isTypeCompatibleWithAtomicOp(valueType,
                                    /*isPointerTypeAllowed=*/true))

Do you want to run the code for atomic loads and stores only? Or are you interested in all loads and stores? For the latter LLVM_LoadableType maybe something to consider. Intuitively, I would have expected that atomics do not support ext type at all?

Regarding the naming supportsMemOps or similar may be a good if it is really about loading and storing?

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
116

nit: Parses -> Parses

Usually all function doc strings should use third person singular.

mlir/test/Target/LLVMIR/Import/target-ext-type.ll
34–46

nit: usually we try to intersperse CHECK lines with the input IR to have the CHECK line as close as possible to the input ir, e.g.:

; CHECK-LABEL: llvm.func @func2()
; CHECK-SAME:      !llvm.target<"spirv.Event"> 
define target("spirv.Event") @func2() {
  ; CHECK-NEXT:    %0 = llvm.mlir.constant(1 : i32) : i32
  ; CHECK-NEXT:    %1 = llvm.mlir.poison : !llvm.target<"spirv.Event">
  ; CHECK-NEXT:    %2 = llvm.alloca %0 x !llvm.target<"spirv.Event"> {alignment = 8 : i64} : (i32) -> !llvm.ptr
  %mem = alloca target("spirv.Event")
  ; CHECK-NEXT:    %3 = llvm.load %2 {alignment = 8 : i64} : !llvm.ptr -> !llvm.target<"spirv.Event">
  %val = load target("spirv.Event"), ptr %mem
  ; CHECK-NEXT:    llvm.return %1 : !llvm.target<"spirv.Event">
  ret target("spirv.Event") poison
}

Improved checking target extension type eligibility for load/store.

Fixed nits in tests and comments.

LukasSommerTu marked 7 inline comments as done.

Add TODOs for 'zeroinitializer'.

@gysit Thanks for your review! I've changed the check for load/store operations and also added the TODOs for the zeroinitializer.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

You're right, target extension types are not allowed in atomic operations, sorry for missing this.

I changed the name of the function as suggested and extended LLVM_LoadableType to include target extension types supporting memory operations.

Thanks we are almost there. I think there is one more check to add for atomics if I understand correctly see my comment below.

Can you maybe also add to brief tests in invalid.mlir that check a load cannot be instantiated with a non-loadable extension type and an atomic cannot be instantiated with a loadable extension type.

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
93

ultra nit: I would probably use LLVM_LoadableTargetExtType to use the same naming as below.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

You're right, target extension types are not allowed in atomic operations, sorry for missing this.

Should we then maybe add a check here that ensures an atomic operation cannot work on a target extension type, i.e.:

if (isa<LLVMTargetExtType>(type))
  return false;
LukasSommerTu marked an inline comment as done.

Fix type constraint. Add tests for invalid use of target extension type.

LukasSommerTu marked an inline comment as done.May 30 2023, 4:42 AM

@gysit Thanks for the review!

I've added the tests as suggested. An explicit check for target extension types wasn't necessary, see my inline comment.

I also needed to change the constraint for LLVM_LoadableType a bit. As target extension types match the LLVM_PrimitiveType.predicate (as they need to return true for isCompatibleOuterType), I had to negate the predicate and renamed it similar to your suggestion (LLVM_NonLoadableTargetExtType). The load example in invalid.mlir checks the error.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

This would already be ensured, as target extension types are neither compatible floating point types nor integer types, so bitWidth would not get a value and false would be returned from:

if (!bitWidth)
    return false;

As the other incompatible types (struct, array, ...) are also not listed explicitly here (and the LLVM verifier logic works similarly), I would not add an explicit check for target extension types, if you agree.

gysit accepted this revision.May 30 2023, 5:08 AM

Thanks LGTM!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
745–746

Ah makes sense!

This revision is now accepted and ready to land.May 30 2023, 5:08 AM

@gysit Thanks for approval. Could you please land this for me, I do not have commit access?

ftynse added inline comments.May 30 2023, 5:29 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
127–128

OptionalParseResult having a value doesn't mean parsing success. It may fail, e.g., if the parsed value would overflow the type.

785

Consider putting these strings into named constexprs.

Check optional parse result. Define name constants.

LukasSommerTu marked 2 inline comments as done.May 30 2023, 5:53 AM

@ftynse Thanks for your feedback, addressed it in the latest update.

ftynse accepted this revision.May 30 2023, 7:01 AM
Dinistro accepted this revision.EditedMay 30 2023, 7:08 AM

I have some final nit comments, apart from that LGTM!

I can fix them and land the commit for you, if you like.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2322

NIT: to be consistent, drop brackets here.

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
120

Ultra nit: This grammar rule is not entirely correct. It should be something like the following:

extTypeParams ::= typeList? | intList? | (typeList "," intList)
typeList      ::= type ("," type)*
intList       ::= integer ("," integer)*

Given that this is quite complicated, it can also be dropped.

781

Ultra NIT: constant are usually prefixed with a "k".

I have some final nit comments, apart from that LGTM!

I can fix them and land the commit for you, if you like.

That would be nice, thanks! If you prefer that I address the nits first, that's also fine, just let me know.

This revision was automatically updated to reflect the committed changes.