This is an archive of the discontinued LLVM Phabricator instance.

[Attributor][FIX] Do not delete non`-mustprogress` calls
Needs ReviewPublic

Authored by jdoerfert on Jan 14 2021, 8:01 PM.

Details

Summary

Even if a call has no side-effects and cannot unwind, we need to
preserve it unless it is mustprogress, termination is a thing now.

Came up as part of D94633, test case included in mustprogress.ll

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 14 2021, 8:01 PM
jdoerfert requested review of this revision.Jan 14 2021, 8:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 8:01 PM
Herald added a subscriber: bbn. · View Herald Transcript
nikic added inline comments.Jan 15 2021, 12:30 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2888

Shouldn't this be checking willreturn rather than mustprogress?

jdoerfert added inline comments.Jan 15 2021, 7:51 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2888

Hm, willreturn is stronger, isn't it (it implies mustprogress IIRC)?
But here we also require no side-effects so they are probably equivalent.
What do you think?

jdoerfert updated this revision to Diff 316964.Jan 15 2021, 8:45 AM

Add more mustprogress/willreturn recursion tests

nikic added inline comments.Jan 15 2021, 12:16 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2888

I think semantically, willreturn is the correct one here. mustprogress works because we assume that mustprogress + readonly = willreturn. However, it would be good to have that knowledge encoded only in one place (willreturn inference), rather than in multiple. Especially as it is not strictly speaking correct, if we wanted to be pedantic about atomic loads (we don't want to though :).

Generally, I think that the mustprogress attribute should only be placed by the frontend, and not derived. Only willreturn should be derived, and that's the property that analyses should be checking.

jdoerfert added inline comments.Jan 15 2021, 12:31 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2888

I think semantically, willreturn is the correct one here. mustprogress works because we assume that mustprogress + readonly = willreturn.

I think I agree. Will change it.

Generally, I think that the mustprogress attribute should only be placed by the frontend, and not derived.

Hm, maybe. It's not that the mustprogress deduction in D94740 actually does much but what it does cannot be replaced by willreturn, I think. Take

static void foo() { ... }
void bar() __attribute__((mustprogress)) { foo() }

We can conclude that foo needs to make progress but it might not return/unwind at all.
Maybe I'm confused again.

Termination is only a side-effect if mustprogress is set, otherwise it is not.
I will rebase this and hope to land it soon after.

Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 4:04 PM