[Codegen] Add control options in pack unpack decomposition #18469
Conversation
Max191
requested review from
MaheshRavishankar,
qedawkins,
kuhar,
Groverkss and
hanhanW
as code owners
|
Depends on llvm/llvm-project#108025 |
| std::unique_ptr<InterfacePass<FunctionOpInterface>> | ||
| createDecomposePackUnPackOpsPass(bool tileOuterToOne); | ||
| createDecomposePackUnPackOpsPass(bool tileOuterToOne, | ||
| bool useOnlyReshapes = false); |
There was a problem hiding this comment.
I don't like the style that one has default values but the other does not. Perhaps we should just retire this wrapper and use the option path, or remove the default values from useOnlyReshapes.
There was a problem hiding this comment.
I realized that we actually need an optional control function on the decomposition as well, so I think it is better to leave the wrapper now. I can add a default value for tileOuterToOne, WDYT?
There was a problem hiding this comment.
Actually I decided to put the control function as an option, since it is optional. The command line option for it will exist but not be usable, but I think it was cleaner this way.
There was a problem hiding this comment.
Nevermind, I just tested this locally and I don't think PassOptions work with functions. I will just leave it with the wrapper.
There was a problem hiding this comment.
I can add a default value for tileOuterToOne, WDYT?
It won't work because it is confusing to the default method. There is already a tablegen-ed definition, and adding a default value for the variable leads to a confusion. You'll likely hit a compilation error. That's why I suggested to remove the default value from useOnlyReshapes,
Max191
force-pushed
the
decompose-pack-with-reshapes
branch
2 times, most recently
from
352dcb0
to
2fdb167
Compare
Max191
changed the title
Max191
force-pushed
the
decompose-pack-with-reshapes
branch
from
2fdb167
to
c326384
Compare
|
(Also wait for approval from Hanhan as well) |
compiler/src/iree/compiler/Codegen/Common/DecomposePackUnPackOps.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Max Dawkins <max.dawkins@gmail.com>
Signed-off-by: Max Dawkins <max.dawkins@gmail.com>
I've tested the change locally on a branch. I'd rather just leave the change in this PR if you don't have a strong opinion, since it is a pretty simple change. |
Signed-off-by: Max Dawkins <max.dawkins@gmail.com>
Max191
force-pushed
the
decompose-pack-with-reshapes
branch
from
c326384
to
e5160bf
Compare
Some early patterns in pack/unpack decomposition avoid generating reshape ops for unit dim packs and unpacks. However, the TileAndFuse pipeline uses these reshape ops to propagate expanded shapes to other fusable ops. This PR adds an option to the DecomposePackUnPackOps pass to create reshape ops anyway for unit dim cases.
The reason these unit dims show up right now is that the iree_linalg_ext.im2col op of a unit-batched conv will have a unit dimension in the batch dim. Ultimately, it would be good to allow for batchless im2col ops, but in general it is good to support ops that have required unit dimensions. When prototyping new ops, it can be easiest to not support rank-reducing cases at first (winograd ops are another example), so these unit dims may appear again in the future.
This PR also adds an optional control function to the pass options, which controls which packs and unpacks get decomposed. The control function is currently expected to be used when the
useOnlyReshapesoption is true, since there is no control function in some upstream patterns yet, but adding the control function upstream and fixing this is left as a TODO.