However, I found that after DCE pass, the intrinsic is removed, as the the intrinsic has readonly attribute, and IntrHasSideEffects doesn’t override IntrReadMem  at all, while in its explaination, it should do so .
So is this expected or a bug of IntrinsicEmitter? If it’s a bug, I’d like to draft a patch to fix it.
In practice, IntrHasSideEffects only overrides IntrNoMem.
I think the bug here is that IntrHasSideEffects exists at all – LLVM IR has no notion of generic “side effects”. Outside of unwinding and divergence, all side effects are modeled via memory. Intrinsics currently using IntrHasSideEffects likely should be modelling this as a read+write of inaccessible memory instead.
Do you mean that I should replace IntrHasSidefEffects with WriteInaccessibleMem? But what if the intrinsic both loads from memory and has sideeffect? Will WriteInaccessibleMem ModRef be confilct with ReadMem?
You are right. It was the other way around, the combo “NoMem + Sideeffect” is “readnone” in the backend (MIR) but not in the IR because the backend uses the special “sideeffect” bit that is not emitted into the IR.
That said, this is not how it should be. We should mark these things with a side effect, and with the new memory attribute we can introduce “categories” to keep the effect on unrelated instructions minimal.
If you look at the original reason the behavior above was introduced (⚙ D64414 Do not set ReadNone attribute on intrinsics with side effects) you’ll see that the “proper” solution would be to model the three transaction intrinsics with effects that only they share, e.g., memory(transaction: write).
Coming back to your original problem. The readnone handling introduced in D64414 should have covered readonly as well. You could do that now, or we properly fix the problem and introduce categories. Another alternative would be to use inaccessiblememonly for now, which will solve the issue and help the backend (albeit less than a category for each of the intrinsics that interact with each other).
@nikic, @fhahn: What do you think about a “flexible” memory location space (which is a bad word for it): memory(space("..."): read/write), such that you can now annotate library functions and intrinsics that interact with each other but not with anything else.
I think to a large part this depends on how much practical use we would get out of a very fine-grained categorization for what is currently “inaccessible memory”. Are we actually missing any significant optimization opportunities due to intrinsics accessing “different” inaccessible memory having spurious dependencies? I’m not aware of any cases where this has been a problem — did you encounter any?
I know places that I’d assume would benefit from it, especially libraries. That said, I don’t have numbers. We can certainly wait for a case study on this first but maybe we get to write the support once, put it on phab, and encourage people to try it out locally to collect numbers. The intrinsic use cases might be something people can measure directly then, e.g., with a benchmark suite for the transaction performance.
I agree that not all side effects are memory related. I literally bring this up a lot. That said, a boolean “has side effect” attribute is not sufficient (IMHO). As for memory, we should acknowledge the different kinds of side effects and group them together in an extensible way. This doesn’t have to be an attribute like memory but could just be the APIs we expose internally. Existing side effects we have right now that are not memory include termination and cross-lane register accesses. Thus, we already have two and probably will encounter more kinds as we go. (I do not include the “thread id” stuff as it should be a memory category (e.g., TLS).) Once memory landed we can revisit this discussion but I hope we end up with a similar side-effect([category]+) attribute that we can use to fix some pesky bugs and enable more optimizations in the process.
I agree it’s pretty coarse-grained, but that’s what we have today as the property in Intrinsics.td yet we don’t properly support it. One could dream up something more advanced, but that’s a feature addition, not a bug fix, IMO.
Just to be clear, when we talk about “memory” in this context, what we really mean is any kind of “state”, which includes but is not limited to ordinary program memory. For example, an intrinsic that reads/writes some kind of special CPU register would model that register state as “memory”, represented as an “inaccessible” location in LLVM’s memory model (or, hypothetically, something more fine-grained, as discussed above).
There are of course other effects that can not be represented as state manipulation, such as the existing unwinding and divergence effects (and I guess convergence, but I’m not familiar with the semantics there). However, these cannot be represented by a generic sideeffect attribute either, because they affect control flow.
It’s not really clear to me what semantics a generic sideeffect attribute would have that makes it distinct from the current memory modelling in terms of permissible optimization. I mean, I can think up semantics that would make it distinct (e.g. an intrinsic that cannot be removed, but can be freely reordered), but those wouldn’t match a typical notion of a side effect.
(The baseline we should be comparing against here is IntrHasSideEffects mapping to an inaccessiblemem: readwrite effect, not the current situation where it is simply completely ignored – I don’t think there’s disagreement that that particular behavior is simply buggy.)
Yes. As mentioned above, we either add inaccessiblememonly to these or we handle readonly like readnone (as introduced in the D64414 workaround).
I don’t think all side-effects should be represented by something that is spelled memory. Purely from the perspective of confusion and existing meanings. That’s why I argued for renaming the memory attribute from the get go into something more generic…
When I think about it I imagine myself explaining to someone what memory effects we have and I can’t think of a way to explain how termination and cross-lane register accesses (which are side-effects we already have in IR) would make sense there. I personally would not consider them “memory” related and I don’t suspect others would, especially since non-cross-lane register accesses are not memory related in this model/lingo.
Heh, so here we are getting into the next layer of hacks. We have a proper way to model that an intrinsic is not going to transfer execution to the next instruction, and that’s the absence of the willreturn attribute (which is part of the default intrinsic properties). It is independent of memory effects, so nominally you can have an intrinsic that is readnone and non-willreturn. That sounds like what you want, if I understood correctly.
To answer your questions directly, no it’s not. Any call(Base) may unwind or not, invoke allows you to catch it though.
To continue my rant: If we would finally setup a proper side-effect API internally we could remove these hacks that allow people to not properly annotate their code but cause problems all over… DefaultIntrinsic, willreturn, noundef, … we introduced most things over the last 2-3 years that we need for this to happen. Let’s remove the hacks and let people update their Intrinsic definitions finally. It’s not that hard.