[PATCH] D76127: [TableGen] Do not set ReadOnly attribute on intrinsics with side effects

Wed Jan 20 13:09:03 PST 2021

jdoerfert added a comment.

In D76127#2510562 <https://reviews.llvm.org/D76127#2510562>, @craig.topper wrote:

> In D76127#2510504 <https://reviews.llvm.org/D76127#2510504>, @jdoerfert wrote:
>
>> TBH, I feel "X is readonly and has side effects" sends the wrong message to begin with. It is a contradiction (in the IR world) as basically shown by the need for this patch. Given that there are no examples in-tree I don't understand why one would mark a side-effect intrinsic as readonly (or similar). Long story short, I would argue this should be a loud error, not silently ignored.
>
> Why shouldn't the intrinsics file be able to express the semantics we want even if we can't represent it in IR today?

My position hasn't changed much from before: https://reviews.llvm.org/D64414#1584449

I mean, the way we interpret the bits for the IR is contradictory, isn't it? In IR, you cannot have an intrinsic that is readonly and has side-effect. Why would we want to express that in the intrinsics file if it is impossible and probably a sign of a misconfiguration. Maybe I misunderstand what "side-effects" are supposed to be here but I find it confusing to say "readonly with side-effects". Interestingly, this "handling" of such a configuration disables middle-end optimizations to enable backend optimizations, which may or may not be worth it.

FWIW, I believe adding more categories other than "inaccessible memory" and "argument memory" is the right way to resolve this issue. Also, side-effect free instructions that are not willreturn should not be deleted in the future. This might not be interesting for the ldmxcsr case (which I don't know what the side-effect is), but for D64414 <https://reviews.llvm.org/D64414> this might be the proper
way to model it.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76127/new/

https://reviews.llvm.org/D76127