[memref] Support dense resources for memref-to-llvm #79380
Conversation
Memref to llvm did not handle the DenseResourceElementsAttr. Reworked the lowering so that it converts to a valid LLVM type.
|
@llvm/pr-subscribers-mlir Author: Rob Suderman (rsuderman) ChangesMemref to llvm did not handle the DenseResourceElementsAttr. Reworked Full diff: https://github.com/llvm/llvm-project/pull/79380.diff 2 Files Affected:
diff --git a/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp b/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
index 2bfca303b5fd48..8fd1f37acdad6c 100644
--- a/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
+++ b/mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
@@ -23,6 +23,7 @@
#include "mlir/Dialect/MemRef/Utils/MemRefUtils.h"
#include "mlir/IR/AffineMap.h"
#include "mlir/IR/BuiltinTypes.h"
+#include "mlir/IR/DialectResourceBlobManager.h"
#include "mlir/IR/IRMapping.h"
#include "mlir/Pass/Pass.h"
#include "mlir/Support/MathExtras.h"
@@ -501,13 +502,21 @@ struct GlobalMemrefOpLowering
Attribute initialValue = nullptr;
if (!global.isExternal() && !global.isUninitialized()) {
- auto elementsAttr = llvm::cast<ElementsAttr>(*global.getInitialValue());
- initialValue = elementsAttr;
-
- // For scalar memrefs, the global variable created is of the element type,
- // so unpack the elements attribute to extract the value.
- if (type.getRank() == 0)
- initialValue = elementsAttr.getSplatValue<Attribute>();
+ Attribute initialAttr = *global.getInitialValue();
+ if (auto resourceAttr = llvm::dyn_cast<DenseResourceElementsAttr>(initialAttr)) {
+ auto blob = resourceAttr.getRawHandle().getBlob();
+ initialAttr = DenseElementsAttr::getFromRawBuffer(
+ resourceAttr.getType(), blob->getData());
+ }
+
+ if (auto elementsAttr = llvm::cast<ElementsAttr>(initialAttr)) {
+ initialValue = elementsAttr;
+
+ // For scalar memrefs, the global variable created is of the element type,
+ // so unpack the elements attribute to extract the value.
+ if (type.getRank() == 0)
+ initialValue = elementsAttr.getSplatValue<Attribute>();
+ }
}
uint64_t alignment = global.getAlignment().value_or(0);
diff --git a/mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir b/mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir
index 37999d6fc14ad1..9e00fd1bb5c73c 100644
--- a/mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir
+++ b/mlir/test/Conversion/MemRefToLLVM/memref-to-llvm.mlir
@@ -330,6 +330,21 @@ memref.global "private" @gv4 : memref<f32> = dense<1.0> {alignment = 64}
// -----
+module {
+ // CHECK: llvm.mlir.global private constant @__constant_xf32(1.41421354 : f32) {addr_space = 0 : i32} : f32
+ memref.global "private" constant @__constant_xf32 : memref<f32> = dense_resource<NAME>
+}
+
+{-#
+ dialect_resources: {
+ builtin: {
+ NAME: "0x08000000F304B53F"
+ }
+ }
+#-}
+
+// -----
+
// Expand shapes need to be expanded outside of the memref-to-llvm pass.
// CHECK-LABEL: func @expand_shape_static(
// CHECK-SAME: %[[ARG:.*]]: memref<{{.*}}>)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Will this work on big endian?
(see recent failure with dense resources support: #78958 (comment) )
So that is a very good question that I am not 100% sure on. I would assume the data storage order should not change between the DenseResourcesAttr as it is just a bag of bytes I would assume it maps to the same way for DenseElementsAttr's byte ordering expectations. Is there a good way to validate? |
|
@uweigand or @Groverkss may be able to help answering here? |
|
This will probably have the same failures because you would interpret the bytes differently on big endian systems and the test will not match. So even if it is correct, your test will fail, because the answer is dependent on endianness. This also needs some tests for non-splat values. |
Non-splat case added. I am a little confused by your comments: are you saying that if endianness matters it will fail no matter what, or even if this test passes it may not be correct. Thanks! |
Thank you for adding the test. I'll try to be more clear. Let's take this test: Here, For a big-endian system, this may be differently interpreted and there would be a different I think we need to disable these tests for big endian systems. |
To be clear: that's a bug though right? |
|
Following #63469 ; I'm wondering if endianness decoding shouldn't be just handled here? Is |
No, it's not a bug. dense_resource does not specify what endianness to use. So the best we can do is assume the endianness of the system (which is what we do). So this test simply has different outputs on a big endian machine. The expectation for dense_resource, AFAIU, is that whoever created it should read it. |
|
I think this is a test bug: dense_resource_elements takes a blob of memory and bitcasts it for access. It is the same as if someone put a char[] literal in their c program and cast it to an array of a multi byte type. It is actually a pretty important part of the contract that intervening code not go mucking with trying to manage/copy it, etc. We use this for capturing raw memory from the framework for jit and related cases, and it is implied that if portability to another endianness is important, that is up to the user to make them agree or convert. This is breaking because the test encodes a little endian blob. To test on big endian, it would need a big endian blob. |
|
I'm not sure I agree just now that this is totally reasonable to serialize non-portable bytecode, without providing any tools to the user to not shoot themselves in the foot like that. |
I think it should be documented. I'm not saying the use case isn't valid for someone and that there shouldn't be an attribute that does more, but that we need one that doesn't have such features -- which put constraints on use and break the memory model. If we don't get that upstream, we'll need to fork our own, most likely. Does llvm bitcode take effort to be portable across machines for its low level constants (as in -- can you load bitcode compiled on one architecture, lto it in another and expect that something has intervened to swap endianness of the data structures at rest)? I don't actually know the answer to that 100% but that is the use case, imo. |
I don't find code that would handle that, so I'd say no (but not with a high confidence) |
|
Probably better to fix closer to the |
Memref to llvm did not handle the DenseResourceElementsAttr. Reworked
the lowering so that it converts to a valid LLVM type.