[PATCH] D78397: [mlir][Transforms] Add pass to perform sparse conditional constant propagation

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
Comment at: mlir/lib/Transforms/SCCP.cpp:313
+
+    // Don't try to fold the results as we can't guarantee folds won't be
+    // in-place.
----------------
rriddle wrote:
> bondhugula wrote:
> > You can with the last output parameter on tryToFold - is this what you were looking for?
> > 
> > ```
> > bool inPlaceUpdate;
> > FoldUtils::tryToFold(op, nullptr, nullptr, &inPlaceUpdate);
> > // operation is folded if inPlaceUpdate is false
> > ```
> This is a bit of a tricky situation. We don't actually want to fold here, this is essentially just simulated execution with constant parameters. The constants we have here aren't guaranteed to be those at runtime, so it's better to avoid the extra overhead of OperationFolder as we don't want to generated anything at this point.
Right - while the algorithm is still running, a current 'constant' lattice value could be later lowered to the bottom symbol ('overdefined'). But once the algorithm has converged, all 'Unknown' lattice values will become either 'constant' or 'overdefined' at which point we could fold the relevant ops. Do you want to collect such ops? You'll still ideally need a new pattern rewriting driver that takes in a list of ops to process as the starting point. In any case, the comment above on "Don't try to fold the results ... won't be in-place" needs to be fixed - it's misleading because tryToFold does in-place updates as well. Instead, change to "can't guarantee that ... will be out of place"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78397