Page MenuHomePhabricator

[IR] Consider non-willreturn as side effect (PR50511)
ClosedPublic

Authored by nikic on Jul 24 2021, 8:44 AM.

Details

Summary

This adjusts mayHaveSideEffect() to return true for !willReturn() instructions. Just like other side-effects, non-willreturn calls (aka "divergence") cannot be removed and cannot be reordered relative to other side effects. This fixes a number of bugs where non-willreturn calls are either incorrectly dropped or moved. In particular, it also fixes the last open problem in https://bugs.llvm.org/show_bug.cgi?id=50511.

I performed a cursory review of all current mayHaveSideEffect() uses, which convinced me that these are indeed the desired semantics. Places that do not want to consider non-willreturn as a sideeffect generally do not want mayHaveSideEffect() semantics at all. I identified two such cases, which are addressed by D106591 and D106742. Finally, there is a use in SCEV for which we don't really have an appropriate API right now -- what it wants is basically "would this be considered forward progress". I've just spelled out the previous semantics there.

Diff Detail

Event Timeline

nikic created this revision.Jul 24 2021, 8:44 AM
nikic requested review of this revision.Jul 24 2021, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2021, 8:44 AM

I like us to check willreturn and consider it an effect. I think we should rename the function though, two reasons:

  1. it changes the behavior and people will be confused.
  2. lang ref talks about "side-effects" and we would need to make it consistent (e.g., for mustprogress)

That said, 2) should be addressed anyway separately anyway. I would not oppose to look for side-effect everywhere and define it
to include termination properly. Does this make sense?

nikic added a comment.Jul 25 2021, 3:44 AM

I like us to check willreturn and consider it an effect. I think we should rename the function though, two reasons:

  1. it changes the behavior and people will be confused.
  2. lang ref talks about "side-effects" and we would need to make it consistent (e.g., for mustprogress)

That said, 2) should be addressed anyway separately anyway. I would not oppose to look for side-effect everywhere and define it
to include termination properly. Does this make sense?

I don't think a rename is necessary here, because the change is a conservatively correct one -- if a use of mayHaveSideEffects() is "incorrect", then at worst that is a performance regression, but it will never cause a miscompile. If we do want to rename it, do you have suggestions on what the name should be? The current "side effect" terminology seems pretty good (you can't add, remove or reorder side effects, which is also true for non-willreturn calls).

I looked at uses of "side effect"/"side-effect" in LangRef, and it seems like the term is currently being used colloquially rather than normatively. We should probably replace uses of this term with a more precise description depending on context. For example "If a readonly function writes memory visible to the program, or has other side-effects [...]" should probably be something like "If a readonly function writes memory visible to the program, or synchronizes [...]".

I initially considered NACK'ing this direction.
It follows a very common practice of stating what the change is,
but not why it is what it is, not why that is the right thing to do.
It would be good to adjust the description accordingly.

nikic retitled this revision from [IR] Consider non-willreturn as side effect to [IR] Consider non-willreturn as side effect (PR50511).Jul 25 2021, 6:06 AM
nikic edited the summary of this revision. (Show Details)
lebedev.ri accepted this revision.Jul 25 2021, 6:29 AM

LG
I will be surprized if this doesn't lead to perf regressions.

This revision is now accepted and ready to land.Jul 25 2021, 6:29 AM
This revision was landed with ongoing or failed builds.Jul 26 2021, 7:35 AM
This revision was automatically updated to reflect the committed changes.

I like us to check willreturn and consider it an effect. I think we should rename the function though, two reasons:

  1. it changes the behavior and people will be confused.
  2. lang ref talks about "side-effects" and we would need to make it consistent (e.g., for mustprogress)

That said, 2) should be addressed anyway separately anyway. I would not oppose to look for side-effect everywhere and define it
to include termination properly. Does this make sense?

I don't think a rename is necessary here, because the change is a conservatively correct one --

correct yes, confusing still, anyway.

If we do want to rename it, do you have suggestions on what the name should be?

good questions, doesn't matter now so let's not try to name things, that's hard anyway.

I looked at uses of "side effect"/"side-effect" in LangRef, and it seems like the term is currently being used colloquially rather than normatively. We should probably replace uses of this term with a more precise description depending on context. For example "If a readonly function writes memory visible to the program, or has other side-effects [...]" should probably be something like "If a readonly function writes memory visible to the program, or synchronizes [...]".

write/readonly/none use it and it is not clear to me if they should now not be applicable to functions with endless loops (I hope they are still as it composes with willreturn).
mustprogress uses it and we can probably rephrase that as it talks about terminating loops without side-effects.