[RFC] Define precise arith semantics

stellaraccident · 2022-09-27T11:54:52.343Z

nlopes:

My suggestion is to introduce poison and freeze in MLIR as standard concepts. And do not introduce undef (see below).

Would poison and freeze go in arith or builtin? I can’t see the proposal for the index dialect yet, so can’t determine if it would be used there (seems like it would). Trying to parse how “standard” this is.

nlopes:

Nevertheless, you can always lower SPIR-V’s OpUndef into freeze poison. That’s correct, and equivalent if freeze ends up with a single use only.

In current pipelines, arith lowers to SPIR-V, which seems the opposite of what is described here?

nlopes · 2022-09-27T12:17:25.618Z

stellaraccident:

nlopes:

My suggestion is to introduce poison and freeze in MLIR as standard concepts. And do not introduce undef (see below).

Would poison and freeze go in arith or builtin? I can’t see the proposal for the index dialect yet, so can’t determine if it would be used there (seems like it would). Trying to parse how “standard” this is.

I don’t know much about MLIR, but probably builtin. You need poison for a lot of things, including as a placeholder for phi nodes, vector creation instructions (insertelement, shufflevector, etc in LLVM).
freeze is needed for optimizations that hoist stuff outside of loops and/or branches and for bit-fields. So it may not be useful for everyone (or maybe no one right now).

stellaraccident:

nlopes:

Nevertheless, you can always lower SPIR-V’s OpUndef into freeze poison. That’s correct, and equivalent if freeze ends up with a single use only.

In current pipelines, arith lowers to SPIR-V, which seems the opposite of what is described here?

Ah, that’s ok as well, as poison can be lowered into undef.

River707 · 2022-09-27T16:13:48.592Z

stellaraccident:

Would poison and freeze go in arith or builtin? I can’t see the proposal for the index dialect yet, so can’t determine if it would be used there (seems like it would). Trying to parse how “standard” this is.

I would somewhat strongly pushback against this going into builtin. It isn’t clear that these are fundamental constructs of building IR, vs useful for modeling UB of specific domains.

Mogball · 2022-09-27T16:28:25.042Z

stellaraccident:

With respect to the tensor baggage specifically, I think we should consider that something to be deprecated and removed. Because the semantics are so unclear and incomplete in those cases, I’m not aware of any practical uses of it (i.e. I’ve seen folks try and then realize that the actual very limited case modeled is of unusably limited utility).

I left a TODO somewhere from when arith was split off to do this but never got to it

clattner · 2022-09-27T16:30:56.529Z

stellaraccident:

clattner:

I’d love to see both things!

Since we’re well on a way to a wishlist, anything else? This stuff only gets harder to fix… Might as well plan to clean it up soon and get it to a long term heathy place.

The other thing on my list are the removal of the ceildiv sorts of operators. These were added to arith as part of lowering from affine, and they are extremely expensive in terms of lowering and thus have completely different cost model from everything else in the arith dialect. IMO, these should be moved to the affine dialect or change affine lowering to generate more primitive arith ops.

I will say that there are theoretical reasons why it could make sense to have these operators somewhere in the system (e.g. range analysis could be used to lower them to cheaper operations) but none of that is implemented in tree AFAIK. Having these in arith is like adding einsum to HLO. I don’t think we would ever have accepted these into arith if they were proposed after arith was established.

Other random thing, why is arith in lib/Dialect/Arithmetic instead of lib/Dialect/Arith ?

-Chris

kuhar · 2022-09-27T16:32:53.009Z

Mogball:

stellaraccident:

With respect to the tensor baggage specifically, I think we should consider that something to be deprecated and removed. Because the semantics are so unclear and incomplete in those cases, I’m not aware of any practical uses of it (i.e. I’ve seen folks try and then realize that the actual very limited case modeled is of unusably limited utility).

I left a TODO somewhere from when arith was split off to do this but never got to it

What are the issues with tensor in arith? I don’t know the historical context.

stellaraccident · 2022-09-27T17:00:39.406Z

River707:

I would somewhat strongly pushback against this going into builtin. It isn’t clear that these are fundamental constructs of building IR, vs useful for modeling UB of specific domains.

(ftr - I have no opinion on this but was responding to the preceding comment about this being “standard” in some way and asking a forcing question)

Mogball · 2022-09-27T17:06:11.447Z

clattner:

I don’t think we would ever have accepted these into arith if they were proposed after arith was established.

They were being proposed at the same time arith was and the understanding was that they would be moved to arith almost immediately.

Also RFC here: [RFC] `index` dialect

kuhar · 2022-09-27T21:11:56.592Z

clattner:

Other random thing, why is arith in lib/Dialect/Arithmetic instead of lib/Dialect/Arith ?

Here you go: ⚙ D134762 [mlir][arith] Change dialect name from Arithmetic to Arith

mehdi_amini · 2022-09-27T21:19:56.122Z

River707:

I would somewhat strongly pushback against this going into builtin. It isn’t clear that these are fundamental constructs of building IR, vs useful for modeling UB of specific domains.

I don’t quite understanding what is “domain specific” about these actually? They seem to be cross-domain and are actually generically defining “a model to manage UB in an optimizing compiler” as far as I understand.
The way I see it is that we need a model to manage UB in the compiler, and upstream we need this for transformations to be able to reason about this and operate safely, regardless of what model we adopt it has to have some central / core support: this seems to me as fundamental as unrealized_cast for example.

stellaraccident · 2022-09-27T21:22:36.944Z

clattner:

The other thing on my list are the removal of the ceildiv sorts of operators. These were added to arith as part of lowering from affine, and they are extremely expensive in terms of lowering and thus have completely different cost model from everything else in the arith dialect. IMO, these should be moved to the affine dialect or change affine lowering to generate more primitive arith ops.

As was noted here, we could just shuttle these to the math dialect now, I think. That dialect shares the characteristics of multiple/complicated lowerings and cost models that are ascribed to these ops. Probably these just got sorted wrong in the original std dialect splitting.

River707 · 2022-09-27T21:29:22.344Z

mehdi_amini:

The way I see it is that we need a model to manage UB in the compiler, and upstream we need this for transformations to be able to reason about this and operate safely, regardless of what model we adopt it has to have some central / core support: this seems to me as fundamental as unrealized_cast for example.

I don’t see why any of this requires these things to be in the builtin dialect, though? Even if it was completely cross domain, we could have a dialect specialized around handling and reasoning about UB. I don’t see why builtin is a requirement for handling things in a “central/core” way. Feels like the same line of thought that led to the standard dialect.

– River

mehdi_amini · 2022-09-27T21:30:16.389Z

Sure, we can introduce a “poison dialect” just to keep these operations around.

River707 · 2022-09-27T21:33:57.317Z

Right. I suppose framed in another way, I’m not against a “blessed” path of handling these things. I’m more of picking on why the “builtin” dialect needs to be the place to house them. We can still have “core” things outside of there that we push as “the way to model/analyze/etc these things”.

clattner · 2022-09-27T21:56:04.328Z

stellaraccident:

As was noted here, we could just shuttle these to the math dialect now, I think. That dialect shares the characteristics of multiple/complicated lowerings and cost models that are ascribed to these ops. Probably these just got sorted wrong in the original std dialect splitting.

Makes perfect sense to me!

kuhar · 2022-09-28T01:28:54.218Z

To summarize the discussion in this and the index RFC thread, I think we are converging towards the following:

Arith needs better defined semantics. These should be separate from LLVM to allow future design freedom on both sides, but LLVM seems like a good reference / starting point for most ops.
To use the LLVM semantics, we need poison / freeze ops. Because this seems more broadly useful, we can put them in a new core-ish dialect, say undefined.
- A dedicated RFC is necessary to cover this in detail.
index support needs to move to a separate dialect.
- This is partially covered by the index dialect RFC (it does not address the removal of index support from arith).
tensor support needs to move out of arith as well:
- A dedicated RFC is necessary to cover this in detail.
n-D vector support may need to move out of arith as well:
- This seems the least clear at this point.

kuhar · 2022-09-28T01:44:22.113Z

You mentioned lowering freeze poison into undef, but can we lower freeze %x into some equivalent instruction sequence under the old LLVM semantics? I’m asking because SPIR-V doesn’t have a freeze op, and I’m not sure if we would have to limit its use to higher-level (pre-spirv) transforms only.

clattner · 2022-09-28T05:28:58.502Z

+1 your summary sounds right to me Jakub!

nlopes · 2022-09-28T08:00:44.317Z

kuhar:

You mentioned lowering freeze poison into undef, but can we lower freeze %x into some equivalent instruction sequence under the old LLVM semantics? I’m asking because SPIR-V doesn’t have a freeze op, and I’m not sure if we would have to limit its use to higher-level (pre-spirv) transforms only.

No, it’s not possible to lower freeze into anything else. That’s why we had to introduce it: there was no equivalent in LLVM before.
A major issue I think is that SPIR-V didn’t define UB carefully. So, we don’t even know what’s the model there. It’s hard for me to make any concrete suggestion (besides revising the SPIR-V spec and making it explicit).

regehr · 2022-10-10T16:18:44.714Z

I don’t disagree with anything that’s been written here, but I’d encourage everyone to learn from LLVM’s mistakes. In particular, over the years there has been a lot of confusion between undef and poison, and (as Nuno implies) it’s pretty difficult to disentangle all this after the fact. So a few things that might help MLIR skip over some of these issues:

do not handwave the semantics!! this is signing up for a very ugly kind of technical debt
lean hard on immediate UB, poison, and freeze poison; avoid undef