[InstCombine] Handle ceil division idiom#100977
[InstCombine] Handle ceil division idiom#100977antoniofrighetto wants to merge 2 commits intollvm:mainfrom
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesThe expression Fixes: #95652. Proof: https://alive2.llvm.org/ce/z/hiWHQA. Full diff: https://github.com/llvm/llvm-project/pull/100977.diff 2 Files Affected:
|
|
Unless I'm missing something, ValueTracking seems to be lacking support of considering the possible overflow coming from *.with.overflow intrinsics, as computeOverflow currently returns MayOverflow when is guaranteed not to overflow. I'll be taking a look. |
| // Fold the log2 ceil idiom: | ||
| // zext (ctpop(A) >u/!= 1) + (ctlz (A, true) ^ (BW - 1)) | ||
| // -> BW - ctlz (A - 1, false) | ||
| const APInt *XorC; |
There was a problem hiding this comment.
Why did you merge the log2 ceil part into this function? I don't think they share common code.
There was a problem hiding this comment.
They do not, but they do both involve the ceil part, thus I thought it was nice to batch them together under a single foldCeilIdioms.
Yeah, ValueTracking doesn't support this assumption. It only exists in some rust applications. See also rust-lang/hashbrown#509. |
There was a problem hiding this comment.
OrZero needs to be false here. (Also comment the constant).
There was a problem hiding this comment.
Technically, I believe it should be allowed, but changed this to false.
There was a problem hiding this comment.
Why does the sub need to be oneuse?
There was a problem hiding this comment.
I just thought it could make sense to restrain the shifted amount as one-use only, although now I realize that N could be used for other log2 calculations, so maybe better to drop it.
|
@nikic Does this pattern exist in real-world code? Can you provide an example without |
|
@dtcxzyw The motivation for this were the regressions we saw from #95556. I think most divideCeil() uses have a constant power of two RHS though, so they will produce a lshr pattern, not the udiv one handled here. But it's possible that we don't actually know that the addition won't overflow in the hot cases. The key one is probably this in DataLayout: This works on uint64_t and nothing tells us that it will not overflow :( |
Would it make sense by any chance to change |
I think a good way to solve this one would be to invert the relationship between getTypeStoreSize and getTypeStoreSizeInBits and use alignToPowerOf2 + plain divide. |
|
#84016 has landed. |
I think we are still missing to preserve the no overflow guarantee. By the time we visit the last addition, both add nuw i8 %x, %y and the assume(no_overflow) are optimized out (the latter is just constant folded to assume(true)). Shouldn't we emit an assume in the form of x+y < uint_max true instead? |
cca259c to
edb1352
Compare
The expression `add (udiv (sub A, Bias), B), Bias` can be folded to `udiv (add A, B - 1), B)` when the sum between `A` and `B` is known not to overflow, and `Bias = A != 0`. Proof: https://alive2.llvm.org/ce/z/hiWHQA.
edb1352 to
0ae6431
Compare
Arguably, this should just happen because WO has no uses (and provided that I'm not missing anything within the AssumptionCache or other ways to query the no overflow guarantee). |
Currently we don't infer no overflow from overflow checking idioms (e.g.,
Agree that we should emit overflow checking idioms instead of To address the motivating case in #95652, we should:
As @nikic pointed out, we have yet to encounter a case for |
|
Thank you for clarifying this! Closing this as I'd rather avoid introducing patterns with dubious real-world usefulness. Happy to reopen it in the future, should it turn out to be needed. |
The expression
add (udiv (sub A, Bias), B), Biascan be folded toudiv (add A, B - 1), B)when the sum betweenAandBis known not to overflow, andBias = A != 0.Fixes: #95652.
Proof: https://alive2.llvm.org/ce/z/hiWHQA.