[PATCH] D106749: [IR] Consider non-willreturn as side effect (PR50511)
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 26 08:01:59 PDT 2021
jdoerfert added a comment.
In D106749#2902911 <https://reviews.llvm.org/D106749#2902911>, @nikic wrote:
> In D106749#2902631 <https://reviews.llvm.org/D106749#2902631>, @jdoerfert wrote:
>
>> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106749/new/
https://reviews.llvm.org/D106749
More information about the llvm-commits
mailing list