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

[Kirill Chibisov]

kchibisov wrote:

What I say that moving `load %a` is fine on its own, unless you move it after the write to `%a`. Which I'd guess `hasSideEffects` can not really solve, since you simply not have access to any context. So the logic should be on the inliner side and inliner must ensure that it doesn't move beyond the `write`'s to the memory location of `%a`, which means that between `load %a` and potential place of `inline` must not be any `write` or call to a function that may write.

So with your original example, it was hard to grasp the issue, I don't see how you can actually break things, since you just move and refer memory location, and you can not speculate with function calls. But if what you do is rely on the `tmp` created by `load` and then use it later on after the `write` to the location of `%a`, then indeed moving it won't going to work, and indeed there's no prevention of any of that, since inliner only acts based of the side `effects`, which assumes that it doesn't touch memory in a meaningful way.

It also means that the current issue is present with the `call`, since it's marked to not have side effects, while it can update global variable IIRC, leading to issues if you read from that variable and you've moved the `call` beyond the `read` of said `global`.

So the problem is that generally the scheduler is not aware of the context over which it tries to move things, and generally should account for **memory writes** between the origin and destination of the moved instruction due to inlining.

So for inliner to work correctly, we should have `MemoryEffects` and move if and only if we don't have any `Write`, but whether to do inlining at all, I'd say that we need a `hasSideEffects` which would be _from the C point of view that could lead to UB/etc,_, where reading variable from stack is generally assumed not do to so.

To sum up, unless the inliner accounts for writes, 1.) said patch can not go 2.) I believe that `call` must be restricted as well and treated as `call_opaque`.

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