This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [sroa] Add support for MemRef.
ClosedPublic

Authored by Moxinilian on May 22 2023, 7:39 AM.

Details

Summary

This patch implements SROA interfaces for MemRef, up to a given fixed
size.

Diff Detail

Event Timeline

Moxinilian created this revision.May 22 2023, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2023, 7:39 AM
Moxinilian requested review of this revision.May 22 2023, 7:39 AM

That looks good to me. I left some nit comments and suggestions.

mlir/lib/Dialect/MemRef/IR/MemRefMemorySlot.cpp
61

nit: I think the call to the ArrayRef constructor can be dropped.

115

nit: can you use MemRefType memrefType = getType(); here? Also I would prefer memrefType or here since it is a type and not a memref value.

140

nit: I think get type can be use directly as well?

auto destructurable = getType().cast<DestructurableTypeInterface>();

189

I would consider renaming to getAttributeIndexFromIndexOperands?

190

nit: Can you use OperandRange or better ValueRange since we are only interested in the values.

194

nit: I believe the namespace is not needed here?

284

nit: I would prefer memrefType.

301

nit: Consider using memrefType as well.

310

Assuming the coordinate is a signed integer I would add a check >= 0 as well.

mlir/test/Dialect/MemRef/sroa.mlir
10

Does CHECK-DAG make sense here? I would assume we can use CHECK: here since it is one dimensional? Otherwise I would go for the same matching solution as in the high dimensional test case?

29

This tests if there is no memref of a different type before right? If we want to check this we should also do this in the other tests e.g. resolve_alias? Also it may be worth considering to use CHECK-NOT: = memref.alloca() : memref<2x2xi32> to make a bit clearer what we are checking for?

30–32
75

nit: CHECK-DAG -> CHECK?

97

nit: CHECK-DAG -> CHECK

137

nit: CHECK-DAG -> CHECK

Moxinilian marked 12 inline comments as done.
Moxinilian edited the summary of this revision. (Show Details)

Address comments.

Moxinilian marked 3 inline comments as done.May 23 2023, 2:04 AM
Moxinilian added inline comments.
mlir/test/Dialect/MemRef/sroa.mlir
29

I'm not sure because I am also checking for absence of any other kind of alloca... But I think it should definitely be everywhere.

Dinistro accepted this revision.May 23 2023, 7:46 AM

LGTM!

This revision is now accepted and ready to land.May 23 2023, 7:46 AM
gysit accepted this revision.May 23 2023, 7:48 AM

LGTM!

mlir/test/Dialect/MemRef/sroa.mlir
29

Sure!

This revision was automatically updated to reflect the committed changes.
mlir/lib/Dialect/MemRef/IR/CMakeLists.txt