Page MenuHomePhabricator

[MLIR] Support memrefs with memref element types.
Needs RevisionPublic

Authored by kernhanda on Feb 19 2020, 1:23 AM.

Details

Summary

With this change, it is now possible to create a memref that's comprised of other memref types.

Diff Detail

Unit TestsFailed

TimeTest
20 msMLIR.IR::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/IR/parser.mlir | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/IR/parser.mlir

Event Timeline

kernhanda created this revision.Feb 19 2020, 1:23 AM
Herald added a project: Restricted Project. · View Herald Transcript

Perhaps memref should be agnostic to the element type it contains? Tensors and Vector types can remain more constrained, but memref should be able to act on many kinds of types, not just the basic fundamental ones.

ftynse requested changes to this revision.Feb 19 2020, 2:13 AM

This is largely insufficient. What is the dynamic representation of such memrefs? How do we lower loads/stores? Do we add type some amount of type erasure (e.g. should it matter that the element type is memref<10xf32> or memref<?xf32> ?)

Don't get me wrong, I think it is important to properly nested pointers for, e.g., sparse storage. They key thing is "properly" though. We need to make sure we are working towards that instead of just relaxing the type constraints on standard types without considering the implications.

Perhaps memref should be agnostic to the element type it contains? Tensors and Vector types can remain more constrained, but memref should be able to act on many kinds of types, not just the basic fundamental ones.

Memref points to a buffer in memory. It does not make sense to have memrefs of a type that don't have a well-defined in-memory representation.

This revision now requires changes to proceed.Feb 19 2020, 2:13 AM

Is there any guidance on what other changes would be required to land this change? I'll write some tests in the meanwhile to ensure that the standard to llvm conversion works well, at least.

nicolasvasilache resigned from this revision.Oct 2 2020, 1:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 1:26 AM