This patch implements SROA interfaces for MemRef, up to a given fixed
size.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
That looks good to me. I left some nit comments and suggestions.
mlir/lib/Dialect/MemRef/IR/MemRefMemorySlot.cpp | ||
---|---|---|
60 | nit: I think the call to the ArrayRef constructor can be dropped. | |
114 | 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. | |
139 | nit: I think get type can be use directly as well? auto destructurable = getType().cast<DestructurableTypeInterface>(); | |
188 | I would consider renaming to getAttributeIndexFromIndexOperands? | |
189 | nit: Can you use OperandRange or better ValueRange since we are only interested in the values. | |
193 | nit: I believe the namespace is not needed here? | |
283 | nit: I would prefer memrefType. | |
300 | nit: Consider using memrefType as well. | |
309 | Assuming the coordinate is a signed integer I would add a check >= 0 as well. | |
mlir/test/Dialect/MemRef/sroa.mlir | ||
9 | 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? | |
28 | 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? | |
29–31 | ||
74 | nit: CHECK-DAG -> CHECK? | |
96 | nit: CHECK-DAG -> CHECK | |
136 | nit: CHECK-DAG -> CHECK |
mlir/test/Dialect/MemRef/sroa.mlir | ||
---|---|---|
28 | 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. |
nit: I think the call to the ArrayRef constructor can be dropped.