Intrinsic with sideeffect is optimized out

Hi all,

I’m trying to add a new intrinsic into llvm, and the intrinsic is lowered to a special intruction, which loads data from memory into a special register.

The instruction is defined as follows,

def MYINST : Instruction {
  let mayLoad = 1;
  let mayStore = 0;
  let hasSideEffects = 1;
  let Defs = [SR]; // SR is a special register

and thus the corresponding intrinsic is defined as follows,

def int_MYINST : Intrinsic<
  [llvm_i32_ty, llvm_i32_ty], // Address and offset

However, I found that after DCE pass, the intrinsic is removed, as the the intrinsic has readonly attribute, and IntrHasSideEffects doesn’t override IntrReadMem [1] at all, while in its explaination, it should do so [2].

So is this expected or a bug of IntrinsicEmitter? If it’s a bug, I’d like to draft a patch to fix it.

[1] llvm-project/IntrinsicEmitter.cpp at main · llvm/llvm-project · GitHub
[2] llvm-project/ at main · llvm/llvm-project · GitHub

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.

Thanks for reply!

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?

Is there appetite for fixing that upstream? We’ve taught LLVM downstream for our own use that you can have side-effects without touching memory.

  1. IntrHasSideEffects is a backend thing. It is only used there IIRC, and for the LLVM-IR it doesn’t make a difference.
  2. I would very much like to have more than just memory effects modeled in LLVM-IR. We have multiple such instances already. As @nikic finishes the memory attribute we can revisit it and maybe rename it into sideeffect, see for example [RFC] Unify memory effect attributes - #5 by jdoerfert

AFAIK, it does have effect on LLVM-IR. As I listed above llvm-project/IntrinsicEmitter.cpp at main · llvm/llvm-project · GitHub, it affects the ReadNone attribute (but not ReadOnly) for intrinsics, and it seems some transformations/analyses use the attribute.

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.

Not all side-effects are memory-related, so I don’t think grouping it under a new memory attribute would make sense. is what I did downstream and IMO should exist upstream.

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 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.

For reference, our use case is assertion intrinsics that will conditionally trap, so not touching any state.

Shouldn’t unwind catch that use case already? (Side note: Our internal APIs don’t consider termination as a side-effect but they also should by default do that already, we have all the tools,…)

What do you mean by unwind? That’s only a thing for asm and invoke?

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.

However, for intrinsics in particular, we currently assume that all readonly intrinsics are also implicitly willreturn: llvm-project/Instruction.cpp at e445349d2cca15cf4581810ce298172a3939c453 · llvm/llvm-project · GitHub

As usual, there is nothing more permanent than a temporary solution…

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.

Hm, !willreturn may well be sufficient these days (it didn’t exist back when I added hassideeffects), provided that readonly->willreturn hack goes away.