Page MenuHomePhabricator

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

Authored by phisiart on Sep 3 2022, 1:34 PM.
This revision needs review, but all reviewers have resigned.

Details

Reviewers
Mogball
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.

Diff Detail

Event Timeline

phisiart created this revision.Sep 3 2022, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 1:34 PM
phisiart requested review of this revision.Sep 3 2022, 1:34 PM
Mogball added a comment.EditedSep 3 2022, 1:47 PM

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

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):
  ...

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

phisiart added a comment.EditedSep 3 2022, 2:14 PM

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.

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

Mogball resigned from this revision.Sep 6 2022, 9:22 AM
phisiart retitled this revision from [mlir][dataflow] Let ConstantValue::dialect also participate in comparison. to [DO NOT SUBMIT] [mlir][dataflow] Let ConstantValue::dialect also participate in comparison..Sep 6 2022, 9:55 AM