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 | ||
---|---|---|
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 |
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. |
nit: I think the call to the ArrayRef constructor can be dropped.