This is an archive of the discontinued LLVM Phabricator instance.

Differential D133434

[mlir][llvm] Support pointer entries in data layout translation
ClosedPublic

Authored by rkayaith on Sep 7 2022, 9:46 AM.

Details

Reviewers
ftynse
dcaballe
nicolasvasilache
Commits
rGaa00e3e6c107: [mlir][llvm] Support pointer entries in data layout translation
Summary

This adds support for pointer DLTI entries in LLVMIR export, e.g.

// translated to: p0:32:64:128
#dlti.dl_entry<!llvm.ptr, dense<[32,64,128]> : vector<3xi32>>
// translated to: p1:32:32:32:64
#dlti.dl_entry<!llvm.ptr<1>, dense<[32,32,32,64]> : vector<4xi32>>

Diff Detail

Event Timeline

rkayaith created this revision.Sep 7 2022, 9:46 AM
Herald added a reviewer: ftynse. · View Herald TranscriptSep 7 2022, 9:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: bzcheeseman, awarzynski, sdasgup3 and 19 others. · View Herald Transcript
rkayaith edited the summary of this revision. (Show Details)Sep 7 2022, 9:47 AM
rkayaith published this revision for review.Sep 7 2022, 10:06 AM
rkayaith added inline comments.
mlir/test/Target/LLVMIR/data-layout.mlir
7

This ignores the idx field of the pointer spec. I'm not sure how to handle that, since we don't support per-addrspace index widths in MLIR. Would it be correct to just set all address spaces to use the type size of index? And how should that work on the import side, when translating datalayout -> DLTI?

Herald added a reviewer: dcaballe. · View Herald TranscriptSep 7 2022, 10:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Harbormaster completed remote builds in B185434: Diff 458484.Sep 7 2022, 10:28 AM
rkayaith added inline comments.Sep 12 2022, 8:31 PM
mlir/test/Target/LLVMIR/data-layout.mlir
7

@ftynse any thoughts here?

ftynse added a comment.Sep 13 2022, 5:28 AM

Could we also add a test with non-default address space to see how it looks in DLTI?

mlir/test/Target/LLVMIR/data-layout.mlir
7

I think we can support the field, at least for roundtripping. At the LLVM dialect level, there is no index type anymore. Any pointer address manipulation that treats them as integers is better off using whatever LLVM expects it to use rather than the converted index type.

rkayaith updated this revision to Diff 459737.Sep 13 2022, 6:36 AM

add test using non-default address space

Harbormaster completed remote builds in B186361: Diff 459737.Sep 13 2022, 7:09 AM
rkayaith updated this revision to Diff 459860.Sep 13 2022, 1:18 PM

Support translating the (optional) 'idx' layout value as well

rkayaith marked an inline comment as done.Sep 13 2022, 1:22 PM
Harbormaster completed remote builds in B186443: Diff 459860.Sep 13 2022, 1:53 PM
rkayaith added inline comments.Sep 15 2022, 10:52 AM
mlir/test/Target/LLVMIR/data-layout.mlir
7

I've added support for exporting the 4th element of dense attribute as the idx field. Looks like the verifier already allowed an optional fourth element so it didn't need any updates, though I didn't see it being used anywhere until now.

I had to make extractPointerSpecValue public to be able to use it during translation.

ftynse accepted this revision.Sep 21 2022, 6:35 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
538 ↗(On Diff #459860)

Nit: carry over the documentation comment from DLEntryPos here.

539 ↗(On Diff #459860)
This revision is now accepted and ready to land.Sep 21 2022, 6:35 AM
Herald added a reviewer: nicolasvasilache. · View Herald TranscriptSep 21 2022, 6:35 AM
Herald added a subscriber: zero9178. · View Herald Transcript
rkayaith updated this revision to Diff 461895.Sep 21 2022, 7:42 AM
rkayaith marked 2 inline comments as done.
rkayaith edited the summary of this revision. (Show Details)

update comments

Harbormaster completed remote builds in B187972: Diff 461895.Sep 21 2022, 7:43 AM
This revision was landed with ongoing or failed builds.Sep 21 2022, 8:16 AM
Closed by commit rGaa00e3e6c107: [mlir][llvm] Support pointer entries in data layout translation (authored by rkayaith). · Explain Why
This revision was automatically updated to reflect the committed changes.
rkayaith added a commit: rGaa00e3e6c107: [mlir][llvm] Support pointer entries in data layout translation.

Revision Contents

PathSize
mlir/
lib/
Target/
LLVMIR/
7 lines
test/
Target/
LLVMIR/
6 lines

Diff 459737

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

Show First 20 Lines • Show All 46 Lines • ▼ Show 20 Lines
using namespace mlir; using namespace mlir;
using namespace mlir::LLVM; using namespace mlir::LLVM;
using namespace mlir::LLVM::detail; using namespace mlir::LLVM::detail;
#include "mlir/Dialect/LLVMIR/LLVMConversionEnumsToLLVM.inc" #include "mlir/Dialect/LLVMIR/LLVMConversionEnumsToLLVM.inc"
/// Translates the given data layout spec attribute to the LLVM IR data layout. /// Translates the given data layout spec attribute to the LLVM IR data layout.
/// Only integer, float and endianness entries are currently supported. /// Only integer, float, pointer and endianness entries are currently supported.
FailureOr<llvm::DataLayout> static FailureOr<llvm::DataLayout>
translateDataLayout(DataLayoutSpecInterface attribute, translateDataLayout(DataLayoutSpecInterface attribute,
const DataLayout &dataLayout, const DataLayout &dataLayout,
Optional<Location> loc = llvm::None) { Optional<Location> loc = llvm::None) {
if (!loc) if (!loc)
loc = UnknownLoc::get(attribute.getContext()); loc = UnknownLoc::get(attribute.getContext());
// Translate the endianness attribute. // Translate the endianness attribute.
std::string llvmDataLayout; std::string llvmDataLayout;
Show All 32 Lines FailureOr<std::string> prefix =
return std::string("i"); return std::string("i");
emitError(*loc) emitError(*loc)
<< "unsupported data layout for non-signless integer " << "unsupported data layout for non-signless integer "
<< integerType; << integerType;
return failure(); return failure();
}) })
.Case<Float16Type, Float32Type, Float64Type, Float80Type, .Case<Float16Type, Float32Type, Float64Type, Float80Type,
Float128Type>([](Type) { return std::string("f"); }) Float128Type>([](Type) { return std::string("f"); })
.Case([](LLVMPointerType ptrType) {
return llvm::formatv("p{0}:", ptrType.getAddressSpace()).str();
})
.Default([loc](Type type) -> FailureOr<std::string> { .Default([loc](Type type) -> FailureOr<std::string> {
emitError(*loc) << "unsupported type in data layout: " << type; emitError(*loc) << "unsupported type in data layout: " << type;
return failure(); return failure();
}); });
if (failed(prefix)) if (failed(prefix))
return failure(); return failure();
unsigned size = dataLayout.getTypeSizeInBits(type); unsigned size = dataLayout.getTypeSizeInBits(type);
▲ Show 20 Lines • Show All 1,058 Lines • Show Last 20 Lines

mlir/test/Target/LLVMIR/data-layout.mlir

// RUN: mlir-translate -mlir-to-llvmir %s -split-input-file -verify-diagnostics | FileCheck %s // RUN: mlir-translate -mlir-to-llvmir %s -split-input-file -verify-diagnostics | FileCheck %s
// CHECK: target datalayout // CHECK: target datalayout
// CHECK: E- // CHECK: E-
// CHECK: i64:64:128 // CHECK: i64:64:128
// CHECK: f80:128:256 // CHECK: f80:128:256
// CHECK: p0:32:64:128
rkayaithAuthorUnsubmitted
Done
ReplyInline Actions

This ignores the idx field of the pointer spec. I'm not sure how to handle that, since we don't support per-addrspace index widths in MLIR. Would it be correct to just set all address spaces to use the type size of index? And how should that work on the import side, when translating datalayout -> DLTI?

rkayaith: This ignores the `idx` field of the [[ https://llvm.org/docs/LangRef.html#data-layout | pointer…
rkayaithAuthorUnsubmitted
Done
ReplyInline Actions

@ftynse any thoughts here?

rkayaith: @ftynse any thoughts here?
ftynseUnsubmitted
Done
ReplyInline Actions

I think we can support the field, at least for roundtripping. At the LLVM dialect level, there is no index type anymore. Any pointer address manipulation that treats them as integers is better off using whatever LLVM expects it to use rather than the converted index type.

ftynse: I think we can support the field, at least for roundtripping. At the LLVM dialect level, there…
rkayaithAuthorUnsubmitted
Done
ReplyInline Actions

I've added support for exporting the 4th element of dense attribute as the idx field. Looks like the verifier already allowed an optional fourth element so it didn't need any updates, though I didn't see it being used anywhere until now.

I had to make extractPointerSpecValue public to be able to use it during translation.

rkayaith: I've added support for exporting the 4th element of dense attribute as the `idx` field. Looks…
// CHECK: p256:256:256
module attributes {dlti.dl_spec = #dlti.dl_spec< module attributes {dlti.dl_spec = #dlti.dl_spec<
#dlti.dl_entry<"dlti.endianness", "big">, #dlti.dl_entry<"dlti.endianness", "big">,
#dlti.dl_entry<index, 64>, #dlti.dl_entry<index, 64>,
#dlti.dl_entry<i64, dense<[64,128]> : vector<2xi32>>, #dlti.dl_entry<i64, dense<[64,128]> : vector<2xi32>>,
#dlti.dl_entry<f80, dense<[128,256]> : vector<2xi32>> #dlti.dl_entry<f80, dense<[128,256]> : vector<2xi32>>,
#dlti.dl_entry<!llvm.ptr, dense<[32,64,128]> : vector<3xi32>>,
#dlti.dl_entry<!llvm.ptr<256>, dense<[256,256,256]> : vector<3xi32>>
>} { >} {
llvm.func @foo() { llvm.func @foo() {
llvm.return llvm.return
} }
} }
// ----- // -----
Show All 18 Lines