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.
Details
Diff Detail
Unit Tests
Event Timeline
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 | ||
---|---|---|
1231–1232 | why don't you want just to remove this match? @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 | ||
---|---|---|
1231–1232 | I want to keep the comment in anyways. |
llvm/test/Transforms/LICM/explicit_guards.ll | ||
---|---|---|
7 | I would move your comment here. This seems to be dedicated test for hoisting. |
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 | ||
---|---|---|
9 | ; Widenable conditions don't actually alias anything or throw, however |
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?