[PATCH] D79502: [mlir] Support partial folding of affine.min/max

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

In D79502#2023690 <https://reviews.llvm.org/D79502#2023690>, @ftynse wrote:

> In D79502#2023207 <https://reviews.llvm.org/D79502#2023207>, @bondhugula wrote:
>
> > > partially folds the affine map by lifting individual constant expression in it
> >
> > Thanks for the detailed summary, but I'm sorry this part isn't clear. canonicalizeMapAndOperands does propagate constants from the operands into the maps. It isn't clear if you are doing the same or whether you are taking min/max over the subset of the results that are constant: for eg. `affine.min affine_map<(d0, d1, d2) -> (d0, d1, d2)> (2, 3, %N)` is  `affine.min affine_map<(d0) -> (2, d0)> (%N)`. I don't see any test cases of the latter form - so didn't look at the code carefully.
>
>
> Canonicalization does more than just propagating the constants, it moves dimensions and symbols around, drops them, or does more aggressive simplifications of affine maps. The folding only looks if it can replace one AffineExpr (however complex) with an AffineConstantExpr by substituting the dimensions and symbols with constant operands. It will do the replacement if possible.


For all the in-place folding for AffineForOp / AffineIfOp, we just make a call to canonicalizeMap/Set/AndOperands and then do the bound folding (the order has to be fixed though - a TODO). On the other hand, these min/max ops have results unlike AffineFor/IfOp and so you are just using the constantFold on the map itself. But SimplifyAffineOp is already registered on min/max ops, and the composeAffineMapAndOperands there is already making a call to canonicalizeMapAndOperands and thus already doing the constant folding for the result expressions - so isn't -canonicalize already accomplishing what you are doing here as part of fold? (albeit via the pattern). You just need the simple min of constants in its folder?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79502/new/

https://reviews.llvm.org/D79502