Page MenuHomePhabricator
Differential D85323 Diff 284945 mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

Changeset View

Standalone View

View Options

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

Show First 20 Lines • Show All 981 Lines • ▼ Show 20 Linesstatic void printGlobalOp(OpAsmPrinter &p, GlobalOp op) {
if (!initializer.empty()) if (!initializer.empty())
p.printRegion(initializer, /*printEntryBlockArgs=*/false); p.printRegion(initializer, /*printEntryBlockArgs=*/false);
} }
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// Verifier for LLVM::DialectCastOp. // Verifier for LLVM::DialectCastOp.
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
static LogicalResult verify(DialectCastOp op) { static LogicalResult verify(DialectCastOp op) {
auto verifyMLIRCastType = [&op](Type type) -> LogicalResult { Type inType = op.in().getType();
ftynseUnsubmitted
Done
ReplyInline Actions

Please don't use auto unless it improves readability (e.g., the RHS has dyn_cast or is a constructor/factory).

ftynse: Please don't use `auto` unless it improves readability (e.g., the RHS has `dyn_cast` or is a…
ftynseUnsubmitted
Done
ReplyInline Actions

Nit: drop trivial braces

ftynse: Nit: drop trivial braces
if (auto llvmType = type.dyn_cast<LLVM::LLVMType>()) { Type outType = op.getType();
if (llvmType.isVectorTy())
ftynseUnsubmitted
Done
ReplyInline Actions

Please terminate sentences with dots in all comments.

ftynse: Please terminate sentences with dots in all comments.
llvmType = llvmType.getVectorElementType(); // Casting between same types is valid.
if (llvmType.isIntegerTy() || llvmType.isBFloatTy() || if (inType == outType)
ftynseUnsubmitted
Not Done
ReplyInline Actions

It's valid but useless, should we add a canonicalization pattern that drops such casts? (not for this commit)

ftynse: It's valid but useless, should we add a canonicalization pattern that drops such casts? (not…
kdobrosAuthorUnsubmitted
Done
ReplyInline Actions

Sure, I actually though there already was such canonicalization since I saw that dialect casts with matching types are removed in lowering from standard.

kdobros: Sure, I actually though there already was such canonicalization since I saw that dialect casts…
llvmType.isHalfTy() || llvmType.isFloatTy() ||
llvmType.isDoubleTy()) {
return success(); return success();
}
return op.emitOpError("type must be non-index integer types, float " auto inLLVMType = inType.dyn_cast<LLVM::LLVMType>();
"types, or vector of mentioned types."); auto outLLVMType = outType.dyn_cast<LLVM::LLVMType>();
}
if (auto vectorType = type.dyn_cast<VectorType>()) { // If types are different then exactly one of them must be LLVM type.
if (!inLLVMType && !outLLVMType)
ftynseUnsubmitted
Done
ReplyInline Actions

Please provide tests for all user-visible error messages.

ftynse: Please provide tests for all user-visible error messages.
return op.emitOpError("incorrect cast between two non-LLVM types");
if (inLLVMType && outLLVMType)
return op.emitOpError("incorrect cast between two different LLVM types");
LLVMType llvmType = inLLVMType ? inLLVMType : outLLVMType;
Type otherType = inLLVMType ? outType : inType;
// TODO: Reduce duplication between below code and LLVMTypeConverter.
auto verifyScalarCastTypes = [](LLVMType llvmType,
Type stdType) -> LogicalResult {
if (stdType.isa<FloatType>())
ftynseUnsubmitted
Done
ReplyInline Actions

This looks like it supports casting f16 to !llvm.double, which isn't desirable. Consider using stdType.isF16()-style functions for stricter checks.

ftynse: This looks like it supports casting `f16` to `!llvm.double`, which isn't desirable. Consider…
return success((llvmType.isBFloatTy() && stdType.isBF16()) ||
ftynseUnsubmitted
Done
ReplyInline Actions

Should we also check the bitwidth of the integer?

ftynse: Should we also check the bitwidth of the integer?
kdobrosAuthorUnsubmitted
Done
ReplyInline Actions

Seems reasonable if we're checking FloatType compatibility.

kdobros: Seems reasonable if we're checking `FloatType` compatibility.
(llvmType.isDoubleTy() && stdType.isF64()) ||
(llvmType.isFloatTy() && stdType.isF32()) ||
(llvmType.isHalfTy() && stdType.isF16()));
if (auto integerType = stdType.dyn_cast<IntegerType>())
return success(llvmType.isIntegerTy() &&
llvmType.getIntegerBitWidth() == integerType.getWidth());
return failure();
};
// Check valid casts in form of allow-list.
if (succeeded(verifyScalarCastTypes(llvmType, otherType)))
return success();
ftynseUnsubmitted
Done
ReplyInline Actions

Don't use else after return.

ftynse: Don't use `else` after `return`.
if (auto vectorType = otherType.dyn_cast<VectorType>()) {
if (vectorType.getShape().size() > 1) if (vectorType.getShape().size() > 1)
return op.emitOpError("only 1-d vector is allowed"); return op.emitOpError("only 1-d vector is allowed");
type = vectorType.getElementType(); if (!llvmType.isVectorTy())
return op.emitOpError("incorrect cast between vector and "
"non-vector types");
LLVMType llvmElementType = llvmType.getVectorElementType();
Type vectorElementType = vectorType.getElementType();
if (failed(verifyScalarCastTypes(llvmElementType, vectorElementType)))
return op.emitOpError("vector elements must have matching "
"non-index integer types or float types");
return success();
} }
if (type.isSignlessIntOrFloat())
if (auto memRefType = otherType.dyn_cast<MemRefType>()) {
ftynseUnsubmitted
Not Done
ReplyInline Actions

Does this handle the special case of function calling conventions? The default convention expands the memref descriptor into multiple types, and we also have the "bare-pointer" convention that uses a plain pointer intsead.

ftynse: Does this handle the special case of function calling conventions? The default convention…
kdobrosAuthorUnsubmitted
Done
ReplyInline Actions

Sorry, I'm not aware where calling convention could interact with dialect cast.
From what I understand irrespective of calling convention all function arguments are immediately packed back into memref descriptor struct.
Only place I guess pointer could be casted to memref is when moving from LLVM to standard and from LLVM comes only this pointer.
For now such case can be only supported with manually creating correct descriptor.
Automatically converting something like this would require adding rewrite pattern (or maybe legalization), so verifier rejects it, at least for now.
Could you maybe explain, if I'm misunderstanding something?

kdobros: Sorry, I'm not aware where calling convention could interact with dialect cast. From what I…
ftynseUnsubmitted
Not Done
ReplyInline Actions

I don't remember offhand how the casts and calling convention interact... The best thing to do is arguably try to run the newly introduced tests with both calling conventions and check that they still pass.

ftynse: I don't remember offhand how the casts and calling convention interact... The best thing to do…
kdobrosAuthorUnsubmitted
Done
ReplyInline Actions

I looked at that first version where separate tests for Standard to LLVM lowering were added.
I saw no difference between calling conventions and memrefs being packed to descriptor struct immediately.

kdobros: I looked at that first version where separate tests for Standard to LLVM lowering were added. I…
Type memRefElementType = memRefType.getElementType();
if (!llvmType.isStructTy())
return op.emitOpError(
"memref can be cast only from/to memref decriptor struct type");
if ((memRefType.getRank() > 0 && llvmType.getStructNumElements() != 5) ||
(memRefType.getRank() == 0 && llvmType.getStructNumElements() != 3))
return op.emitOpError(
"incorrect number of elements in memref descriptor struct");
LLVMType allocatedPtrType = llvmType.getStructElementType(0);
LLVMType alignedPtrType = llvmType.getStructElementType(1);
if (!allocatedPtrType.isPointerTy() || !alignedPtrType.isPointerTy())
return op.emitOpError("first and second element of memref descriptor "
"must have pointer type");
LLVMType allocatedElementType = allocatedPtrType.getPointerElementTy();
LLVMType alignedElementType = alignedPtrType.getPointerElementTy();
if (allocatedElementType != alignedElementType ||
failed(verifyScalarCastTypes(allocatedElementType, memRefElementType)))
return op.emitOpError(
"first and second element of memref descriptor must have same types, "
"that is pointer to type convertible to memref's element type");
// For now skip checking other fields of memref descriptor as they rely on
// index lowering.
ftynseUnsubmitted
Done
ReplyInline Actions

Consider inverting the condition and returning early to decrease horizontal indentation.

ftynse: Consider inverting the condition and returning early to decrease horizontal indentation.
return success(); return success();
// Note that memrefs are not supported. We currently don't have a use case }
// for it, but even if we do, there are challenges:
// * if we allow memrefs to cast from/to memref descriptors, then the return op.emitOpError("incorrect cast").attachNote()
// semantics of the cast op depends on the implementation detail of the << "casting is supported for non-index integers and floats, "
// descriptor. "vectors and memrefs of those";
// * if we allow memrefs to cast from/to bare pointers, some users might
// alternatively want metadata that only present in the descriptor.
//
// TODO: re-evaluate the memref cast design when it's needed.
return op.emitOpError("type must be non-index integer types, float types, "
"or vector of mentioned types.");
};
return failure(failed(verifyMLIRCastType(op.in().getType())) ||
failed(verifyMLIRCastType(op.getType())));
} }
ftynseUnsubmitted
Not Done
ReplyInline Actions

can we do something like

op.emitOpError("incorrect cast").attachNote("casting is supported for integers and floats, vectors and memrefs thereof");

that is more concise and decouples the error message from the additional explanation?

ftynse: can we do something like op.emitOpError("incorrect cast").attachNote("casting is supported for…
kdobrosAuthorUnsubmitted
Done
ReplyInline Actions

Sure, changed.
I haven't noticed that there is this attachNote method.

kdobros: Sure, changed. I haven't noticed that there is this `attachNote` method.
// Parses one of the keywords provided in the list `keywords` and returns the // Parses one of the keywords provided in the list `keywords` and returns the
// position of the parsed keyword in the list. If none of the keywords from the // position of the parsed keyword in the list. If none of the keywords from the
// list is parsed, returns -1. // list is parsed, returns -1.
static int parseOptionalKeywordAlternative(OpAsmParser &parser, static int parseOptionalKeywordAlternative(OpAsmParser &parser,
ArrayRef<StringRef> keywords) { ArrayRef<StringRef> keywords) {
for (auto en : llvm::enumerate(keywords)) { for (auto en : llvm::enumerate(keywords)) {
if (succeeded(parser.parseOptionalKeyword(en.value()))) if (succeeded(parser.parseOptionalKeyword(en.value())))
▲ Show 20 Lines • Show All 1,044 Lines • Show Last 20 Lines