This adds AllocaOp to the operand ops that we support for folding.
This folding can be very useful for extent tensor computations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
40 ms | x64 debian > Flang.Semantics::resolve102.f90 |
Event Timeline
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1537 | You are completely right, it doesn't have to be a rank op. This was my motivating example, but no need to restrict it to that. |
mlir/test/Dialect/Standard/canonicalize.mlir | ||
---|---|---|
137–146 | This folding already exists and happens upstream on this test case. Is this an (extra) test case for existing functionality? The fold pattern on DimOp already handles this for any dimensionality memrefs. | |
155 | Please change the name of this function - it gives the impression that this has something to do with the RankOp but in reality it has nothing to. It could as well be the result of any operation or even a block argument. Also, this can be done for any dimensional memref. | |
157 | This is really a folding pattern and should be implemented as part of DimOp::fold - it allows faster progress in the canonicalizer (greedy rewrite driver) and that's the standard approach. All such replacements on other ops are actually implemented on the folding hook - some that were canonicalization patterns earlier were migrated almost a year ago to the folding patterns. The fold methods are also called during the greedy rewrite driver. It just allows more and earlier simplification with this being part of fold. |
mlir/test/Dialect/Standard/canonicalize.mlir | ||
---|---|---|
137–146 | You are right, I just hadn't seen that this already works. I started with noticing that dim(alloca) of a 1D memref was not canonicalized, and that was what I initially wanted to fix. I removed the test case again. | |
157 | Thanks, that makes sense :) |
A couple more comments @akuegel
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1501–1505 | Can these two ifs be combined into a single one with the dyn_cast_or_null on AllocLikeOp? | |
mlir/test/Dialect/Standard/canonicalize.mlir | ||
147 | Could you add extra lines to test the case when the alloc operand is a block argument? That should just fold as well. |
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1447–1448 | I still don't understand what the connection with rank 1 is. This folding for DimOp or the propagation of the AllocLikeOp operand can happen for any ranked memref operand of a DimOp. |
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1448 | if the code is correct <- This would be a bit misleading. I think you are referring to undefined behavior which would happen if the index of a dim op is >= rank of memref/tensor. Basically, you would be exploiting undefined behavior. if the code is correct -> for the behavior to be defined ? @mehdi_amini @rriddle Should we exploiting undefined behavior this way? | |
1450–1455 | I think I now see what you were handling and the other thing I was thinking about. I was just thinking (separately from the intention of your patch) that you could get the indexth operand from the AllocLikeOp that's defining this memref and fold the DimOp by replacing its result with that value. A Value is returned as OpFoldResult in such cases. It is orthogonal to what you are doing here. |
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1448 | I thought about this a bit more, and even if we allow exploiting undefined behavior (which seems to be not clear), it is not worth it. If the emitted code knows it is dealing with a 1D memref, I think the index operand of DimOp would always be a constant. So I removed this part again. | |
1450–1455 | If it is orthogonal, I guess it could be done in a separate patch. | |
1501–1505 | As far as I can tell, the class AllocLikeOp only exists in the Ops.td file, it is not generated as part of the code created with tablegen. If I look at the generated AllocOp class, it derives from ::mlir::Op, not from AllocLikeOp. But I am not too familiar with the MLIR infrastructure yet, so it is possible I overlooked something. |
LGTM.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp | ||
---|---|---|
1501–1505 | Does it compile when you use AllocLikeOp? Yes, it only appears in the td file. A class doesn't have to inherit from another for cast/dyn_cast etc. to work. (For eg. Operation * cast to any derived OpTy.) |
Does it compile when you use AllocLikeOp? Yes, it only appears in the td file. A class doesn't have to inherit from another for cast/dyn_cast etc. to work. (For eg. Operation * cast to any derived OpTy.)
Unfortunately it doesn't compile. No C++ class seems to be generated for it (I didn't find it in the Ops.h.inc (now MemRefOps.h.inc) file where e.g. AllocOp and AllocaOp classes are forward declared). Maybe it would be generated in some other file, and I would need to include that other file?
I still don't understand what the connection with rank 1 is. This folding for DimOp or the propagation of the AllocLikeOp operand can happen for any ranked memref operand of a DimOp.