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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2107
+
+  results.reserve(map.getNumResults());
+  for (AffineExpr expr : map.getResults()) {
----------------
nicolasvasilache wrote:
> How about a 2 steps impl?
> 
> ```
> if (llvm::any_of(...))
>   return failure();
> auto range = llvm::map_range();
> results.assign(range.begin(), range.end());
> ``` 
> 
> I'd say the readability improvements are worth it, you choose.
I tried, and it's actually more verbose because of lambdas.


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2162
+  // If some of the map results are not constant, try changing the map in-place.
+  SmallVector<int64_t, 2> results;
+  if (failed(extractIntegerResults(foldedMap, results))) {
----------------
nicolasvasilache wrote:
> andydavis1 wrote:
> > Seems like this block of code could be shared with AffineMin::Fold
> Or have the whole impl factored out and just take either `std::min/max_element` as a parameter?
taking an instantiation of an STL algorithm as a parameter looks very ugly, so I just went for an if(std::is_same)


================
Comment at: mlir/lib/IR/AffineMap.cpp:242
+  // containing those constants.
+  results.reserve(newMap.getNumResults());
+  for (AffineExpr expr : newMap.getResults()) {
----------------
nicolasvasilache wrote:
> Same potential 2-step readability gains as above.
It's even better with the refactoring suggested by Andy below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79502