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.