[Mlir-commits] [mlir] [MLIR][SideEffects][MemoryEffects] Modified LICM to be more aggressive when checking movability of ops with MemWrite effects (PR #155344)

[Mehdi Amini]

================
@@ -347,18 +347,39 @@ struct AlwaysSpeculatableImplTrait
 //===----------------------------------------------------------------------===//
 
 namespace MemoryEffects {
-/// Defines the priority of the different memory effects.
+/// Defines the expected evaluation priority/order of the different Memory
+/// Effects for when two Effect instances on the same op are defined with the
+/// same Stage.
 ///
-/// Sorting/ordering memory effects of an operation is done based on
-/// their defined stage and priority, in that order. If stage values for two
-/// effect instances are equal, they are then sorted by priority. Lower priority
-/// values indicate higher precedence.
+/// This adds a concrete evaluation priority to the Effect instances to allow us
+/// to analyze potential Memory Effect conflicts in a transparent, predictable,
+/// and deterministic manner AND creates a better defined contract with users.
+/// i.e., a set of Memory Effect instances on an op could look like the
+/// following: [MemRead<rA, 0>, MemFree<rA, 0>] where the evaluation order is
+/// ambiguous based on how the user defined both effects with the same Stage 0.
+/// If we Read then Free, we are safe. If we Free first, then we'll Read
+/// garbage. Reordering the effects based on priority removes this ambiguity.
+/// Stage values can be leveraged by users to define the exact order that the
+/// Effect instances take place.
+///
+/// When sorting an op's MemoryEffects, we first sort the effect instances
+/// in increasing Stage order. If Stage values for two Effect instances are
+/// equal, they are then sorted in order of decreasing priority, as defined
+/// by this enum. Higher priority values indicate higher precedence.
----------------
joker-eph wrote:

I'm very confused: why do we need another mechanism than stages? 
Stages is the way users can order effects, if they put them in the same stage that means they don't want to order them, yet you're reintroducing artificially a forced ordering under the hood.

This is also IMO making it more likely for people to have bugs because now there is an implicit ordering that they have to keep in mind.


https://github.com/llvm/llvm-project/pull/155344