[Mlir-commits] [mlir] [mlir][emitc] Relax `hasSideEffect` rules for Var/Member (PR #145011)

[Gil Rapaport]

aniragil wrote:

> > consider (_(wrong_struct_)pointer_to_int).field_out_of_bounds).
> 
> Wouldn't it break because of the `deref` in the chain when talking about `mlir` semantics? Like `deref` would force a load of `struct` those copy and member will operate on the loaded copy?

Yes, my example is indeed a bit problematic as the `apply "*"` we currently have in EmitC predates the lvalue semantics we added and should be redesigned to return an `lvalue` type which can then be loaded from (RHS) or assigned to (LHS), as C's `*` dereference operator behaves.

> Just to ensure, are you talking about moving of the `load` instructions itself and not about anything related to `CExpression`? Since I'd assume that what you're talking about is speculative change of order of operations here. If that's the case, isn't `MemoryEffects` is what should be used for that? The `HasSideEffects` from my point of view is a hand rolled concept to better model `C` side effects to allow better merging of operations (which is what I care about, to have more things merged and look more natural for C code). From the `MLIR`'s not emitc related passes perspective, the `MLIR` original interface should be used for that, which is not utilized right now at all.

Actually I do mean that in the CExpression/`emitc.expression` aspect, as both the form-expressions pass and the translator rely on the `hasSideEffect()` API to avoid merging/inlining expressions which are not safe to move speculatively for whatever reason (`emitc.expression`'s only purpose is to provide better-looking C code).
We should definitely refine form-expression's merge logic to merge load and stores where possible, and we should use [MLIR's existing side effect interfaces](https://mlir.llvm.org/docs/Rationale/SideEffectsAndSpeculation/) for that, but we'd still need to make sure we're not changing memory semantics when moving ops.

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