This is an archive of the discontinued LLVM Phabricator instance.

[LICM] Do not hoist widenable conditions
ClosedPublic

Authored by mkazantsev on Mar 17 2023, 12:33 AM.

Details

Summary

Despite the fact that it is legal, it is not profitable. Because of bug described at
https://github.com/llvm/llvm-project/issues/60234, now the guard widening is
only possible when condtion we want to add is available at the point of the
widenable_condition() of dominating guard. It means that, if all such calls are
hoisted out of loop, and the loop conditions depend on loop-variants, we cannot
widen. Hoisting is otherwise not helpful, because it does not introduce any
optimization opportunities.

Diff Detail

Event Timeline

mkazantsev created this revision.Mar 17 2023, 12:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 12:33 AM
mkazantsev requested review of this revision.Mar 17 2023, 12:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 12:33 AM
skatkov added a subscriber: apilipenko.

The change itself looks reasonable.

However it would be interesting to know the motivation this check has been added.

llvm/lib/Transforms/Scalar/LICM.cpp
1232

why don't you want just to remove this match?
In my understanding by default LICM will touch it...

@apilipenko, this match has been added at https://reviews.llvm.org/D69907. Do you know any specific motivation to allow hoisting of widenable condition intrinsic? May be anything is missing?

Hoisting was added in D69907. @apilipenko @reames do you agree it's a good enough reason to not do it?

llvm/lib/Transforms/Scalar/LICM.cpp
1232

I want to keep the comment in anyways.

mkazantsev marked an inline comment as done.

Removed check, comment moved to test file.

skatkov added inline comments.Mar 17 2023, 1:12 AM
llvm/test/Transforms/LICM/explicit_guards.ll
7

I would move your comment here. This seems to be dedicated test for hoisting.

mkazantsev marked an inline comment as done.

Moved comment to other file.

apilipenko accepted this revision.Mar 17 2023, 2:01 PM

Looks good to me. I don't think we had a specific motivation in mind for that change. And now we know a clear reason not to LICM widenable conditions.

llvm/test/Transforms/LICM/explicit_guards.ll
10

; Widenable conditions don't actually alias anything or throw, however
; hoisting them is not profitable because it may prevent guard widening in the loop.

This revision is now accepted and ready to land.Mar 17 2023, 2:01 PM
mkazantsev marked an inline comment as done.Mar 19 2023, 10:22 PM
This revision was landed with ongoing or failed builds.Mar 19 2023, 10:29 PM
This revision was automatically updated to reflect the committed changes.