In the absence of maps, we can lower memref.copy to a memcpy.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Do you want to instead create one pattern with two functions in it? It'll lead to less overhead in the greedy rewrite driver and perhaps also easier to later choose between the two.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp | ||
---|---|---|
781 | Doc comment here - along the lines of your revision summary and title. |
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp | ||
---|---|---|
727 | This should likely be documented, I think notifyFailure would do it here. |
I though about that, too. Another way would be to give the memcpy based pattern higher benefit and actually make then non-exclusive. That way one could optionally blend in the memcpy based pattern.
One issue I currently see with this entire thing is that it relies on maps being present as a way to understand whether the memref has identity strides and offset. I am not sure whether this assumption actually holds for all users.
All of this still leads to two patterns.
One issue I currently see with this entire thing is that it relies on maps being present as a way to understand whether the memref has identity strides and offset. I am not sure whether this assumption actually holds for all users.
You are just using isIdentity() -- the API doesn't name map outside. In the inside, it's still a map.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp | ||
---|---|---|
710–711 | non-identity maps -> non-identity layout There is no reference to map in the API and in your code below and there isn't a need to. The MemRefLayoutAttrInterface hides it. |
Yes, my question is more whether that is useful, to have it as two patterns vs. one. The benefit modelling would be one reason. If everybody thinks that this is not useful enough for the cost, I can merge them.
One issue I currently see with this entire thing is that it relies on maps being present as a way to understand whether the memref has identity strides and offset. I am not sure whether this assumption actually holds for all users.
You are just using isIdentity() -- the API doesn't name map outside. In the inside, it's still a map.
I was not clear. I was wondering about the case where the static type of the memref does not have a map but the descriptor at runtime still does not have identity strides. Currently, one can create this setting using memref.reinterpret_cast by omitting the map from the target type. Maybe that should be illegal.
You are just using isIdentity() -- the API doesn't name map outside. In the inside, it's still a map.
I was not clear. I was wondering about the case where the static type of the memref does not have a map but the descriptor at runtime still does not have identity strides. Currently, one can create this setting using memref.reinterpret_cast by omitting the map from the target type. Maybe that should be illegal.
This would be an invalid op and should have failed the verifier in the first place. (Note that the memref always has a map -- it's not printed if it's identity as you know. Also, identity strides (all ones) don't correspond to an identity map -- N^2, N, 1 for example would correspond to an identity map for a 3-d memref for example.)
I have sent https://reviews.llvm.org/D116601 to improve verification. One can still create this case dynamically but as we consider that illegal, the result of memref.copy is undefined in such cases, so it is fine to just use the intrinsics.
non-identity maps -> non-identity layout
There is no reference to map in the API and in your code below and there isn't a need to. The MemRefLayoutAttrInterface hides it.