Page MenuHomePhabricator

[mlir][arith] Change dialect name from Arithmetic to Arith
ClosedPublic

Authored by kuhar on Sep 27 2022, 2:10 PM.

Details

Summary

Suggested by @lattner in https://discourse.llvm.org/t/rfc-define-precise-arith-semantics/65507/22.

Tested with:
ninja check-mlir check-mlir-integration check-mlir-mlir-spirv-cpu-runner check-mlir-mlir-vulkan-runner check-mlir-examples

and bazel build --config=generic_clang @llvm-project//mlir:all.

Diff Detail

Event Timeline

kuhar created this revision.Sep 27 2022, 2:10 PM
kuhar requested review of this revision.Sep 27 2022, 2:10 PM
lattner accepted this revision.Sep 27 2022, 2:52 PM

+1 this makes sense to me, but I'd love for someone else's eyes to be on the details. Thank you!

Mogball requested changes to this revision.Sep 27 2022, 5:15 PM

I'm against this change. The dialect has always been called "the arithmetic dialect", which everyone just called "arith" for short. There are few other dialects whose name and namespaces don't match; "omp" for example

This revision now requires changes to proceed.Sep 27 2022, 5:15 PM

Is there a reason to have two names for the same thing? Why shouldn't the omp dialect be moved into a directory "omp"?

People can still verbally refer to these things as openmp and arithmetic of course, but keeping the code mechanically consistent seems just better?

kuhar added a comment.Sep 27 2022, 6:09 PM

I'm against this change. The dialect has always been called "the arithmetic dialect", which everyone just called "arith" for short. There are few other dialects whose name and namespaces don't match; "omp" for example

Without knowing the full this historical context, the current state of things seems needlessly inconsistent to me. For example, we have Math and Func instead of Mathematical/Function. Another example is that recently, we renamed changed all definitions in the SPIRV dialect from 'spv' to spirv for the sake of consistency. If the refactoring cost is low, I'd rather use the same dialect names and definition/op prefix.

Mogball accepted this revision.Sep 27 2022, 6:10 PM

I wasn't aware of the spv change. LGTM to them

This revision now requires review to proceed.Sep 27 2022, 6:10 PM
rriddle accepted this revision.Sep 27 2022, 6:17 PM

I only really hear this referred to as "Arith", so this rename seems good to me.

csigg added inline comments.Sep 28 2022, 12:36 AM
mlir/include/mlir/Dialect/Arith/IR/Arith.h
24

Could we have a type alias (using ArithmeticDialect = ArithDialect;) to help with the transition?

Maybe you already added it somewhere, I haven't checked all the files.

kuhar added a comment.Sep 28 2022, 8:05 AM

Based on internal feedback, I want to split this patch into a few more gradual changes:

  1. (This revision) Rename the dialect and paths, but add an alias for the old dialect name
  2. Remove the alias
  3. Rename passes/conversions (landed a few days later so that folks have time to catch up)

Does this work for other folks?

mlir/include/mlir/Dialect/Arith/IR/Arith.h
24

Sure, I can add this if it makes it easier to import this change

cota added a comment.Sep 28 2022, 10:03 AM

Based on internal feedback, I want to split this patch into a few more gradual changes:

  1. (This revision) Rename the dialect and paths, but add an alias for the old dialect name
  2. Remove the alias
  3. Rename passes/conversions (landed a few days later so that folks have time to catch up)

Does this work for other folks?

An alternative, that is easier for you @kuhar as well as simpler for upstream, is just to land the patch as is, i.e. in a single commit that does the entire conversion.

This patch is large but the changes are quite mechanical, so perhaps splitting it into several patches is overkill. I remember the func -> func.func change was quite wide-ranging and therefore required a lot of work, but in this case we should be able to adapt downstream code relatively easily, even if it all comes in a single patch.

So, as a downstream user, I'd be happy with or without the split.

jpienaar accepted this revision.Sep 28 2022, 10:10 AM

LG for shape changes/removing blocker

This revision is now accepted and ready to land.Sep 28 2022, 10:10 AM

OK, so if I don't get any other requests to split this up, I'll land this as-is in one go tomorrow morning (Pacific Time).

tpopp added a subscriber: tpopp.Sep 29 2022, 12:39 AM
tpopp added inline comments.
mlir/include/mlir/Conversion/Passes.td
98–99

Just a warning that you'll have merge conflicts with this change: https://reviews.llvm.org/D134752

Based on internal feedback, I want to split this patch into a few more gradual changes:

  1. (This revision) Rename the dialect and paths, but add an alias for the old dialect name
  2. Remove the alias
  3. Rename passes/conversions (landed a few days later so that folks have time to catch up)

Does this work for other folks?

Seems reasonable to me to splits.

I am not sure about the “ a few days later so that folks have time to catch up)” part, that seems oddly specific to “some specific downstream” in terms of process/timeline (what if I integrate weekly? Every other week? …)

mehdi_amini accepted this revision.Sep 29 2022, 7:54 AM

And thanks for the cleanup by the way :)

kuhar updated this revision to Diff 463912.Sep 29 2022, 8:12 AM

Rebased.

kuhar edited the summary of this revision. (Show Details)Sep 29 2022, 8:13 AM
This revision was landed with ongoing or failed builds.Sep 29 2022, 8:24 AM
This revision was automatically updated to reflect the committed changes.

This is breaking couple of flang buildbots.

This is breaking couple of flang buildbots.

That should be fixed by https://reviews.llvm.org/rG6c8d8d10455a3d38a92102ae3a94e1c06d3eb363. I haven't received any builbot errors since submitting this fix, so I think that should do it. I'm not a flang developer, so please let me know if there are any remaining issues.

This is breaking couple of flang buildbots.

That should be fixed by https://reviews.llvm.org/rG6c8d8d10455a3d38a92102ae3a94e1c06d3eb363. I haven't received any builbot errors since submitting this fix, so I think that should do it. I'm not a flang developer, so please let me know if there are any remaining issues.

Thanks for the quick fix. I think everything is fine.