[DO NOT SUBMIT] [mlir][dataflow] Let ConstantValue::dialect also participate in comparison.

[phisiart]

Summary

ConstantValue is defined as { Attribute constant; Dialect *dialect; }.

ConstantValue::dialect specifies how we can materialize the Attribute contant into an op. If two ConstantValues have the same constant but different dialects, then they are still unequal because they would materialize into different ops. This means that joining them still results in an unknown constant.

[Mogball]

If two ConstantValues have the same constant but different dialects, then they are still unequal because they would materialize into different ops.

This isn't true. Most dialects upstream (and many downstream) use arith.constant.

This patch will unnecessarily impede hybrid dialect IRs from behaving nicely in SCCP

[phisiart]

In D133260#3768806, @Mogball wrote:

If two ConstantValues have the same constant but different dialects, then they are still unequal because they would materialize into different ops.

This isn't true. Most dialects upstream (and many downstream) use arith.constant.

This patch will unnecessarily impede hybrid dialect IRs from behaving nicely in SCCP

You are right. I'm dropping this patch. However, how do you handle the case where you want to join constants from different dialects?

^bb1:
  %const_1 = arith.constant {1}
  cf.br ^bb_join(%const_1)

^bb2:
  %const_2 = my_own_dialect.my_own_constant {2}
  cf.br ^bb_join(%const_2)

^bb_join(%arg):
  ...

[Mogball]

In this case, the actual values differ (1 vs 2) so they won't fold

[phisiart]

In D133260#3768847, @Mogball wrote:

In this case, the actual values differ (1 vs 2) so they won't fold

Ah sorry! I meant:

^bb1:
  %const_1 = arith.constant {1}
  cf.br ^bb_join(%const_1)

^bb2:
  %const_2 = my_own_dialect.my_own_constant {1}
  cf.br ^bb_join(%const_2)

^bb_join(%arg):
  ...

In this case, %const_1's ConstantValue state is { constant = 1, dialect = arith }, and %const_2's ConstantValue state is { constant = 1, dialect = my_own_dialect }.

Then, we join these two ConstantValues. Since we only compare ConstantValue::constant in operator==, the result of the join is { constant = 1, dialect = arith } (since join returns lhs if lhs == rhs). Then SCCP would think that %arg is equal to arith.constant {1}.

However, I think that in this code example, we shouldn't be able to join, because %const_1 and %const_2 are different.

[Mogball]

In that case, one of the dialects will get picked (arith in this case). This can be problematic if the materialized constants could have different semantics, but it's not clear how semantics are define. All constant ops are side effect free and so it sort of shouldn't matter which op gets used. Even if you fold arith.constant 1 : i32 and llvm.mlir.constant(1 : i32) : i32, the former lowers to the latter anyways.

The problem arises when, say, one constant op places a constant in one kind of register and another does a different register. We can be clever and inspect the actual materialized op and require they be the same for both dialects. In the arith vs llvm case above, we might just say that sccp needs to run again after arith is lowered to llvm

mlir/include/mlir/Analysis/DataFlow/ConstantPropagationAnalysis.h

public:
   /// Get the constant value. Returns null if no value was determined.
   Attribute getConstantValue() const { return constant; }
 
   /// Get the dialect instance that can be used to materialize the constant.
   Dialect *getConstantDialect() const { return dialect; }
 
   /// Compare the constant values.
   bool operator==(const ConstantValue &rhs) const {
-    return constant == rhs.constant;
+    return constant == rhs.constant && dialect == rhs.dialect;
   }
 
   /// Print the constant value.
   void print(raw_ostream &os) const;
 
   /// The state where the constant value is unknown.
   static ConstantValue getUnknownConstant() { return {}; }