Move CVP before instcombine in the pass pipeline #48110
Conversation
I was investigating why LLVM was unable to remove the boundscheck that was causing the test failure in #48066 and it turned out that the check was removable by instcombine, but only if cvp ran first. However, we ran the passes in the opposite order and then loop-rotated the condition away before instcombine ran again. I think it makes sense to run cvp before instcombine to make sure that instcombine can make use of the overflow flags inferred by cvp. For the particular case in #48066, we also need https://reviews.llvm.org/D140933 (and we may need it in general, since we like to emit our conditionals as negations), but overall this seems like a better place for cvp in our pass pipeline.
|
Looks like we'll need to merge https://reviews.llvm.org/D140933 first before we can turn this on. |
|
Seems like the indvars pipeline is rather unhappy with this change. Also, we should keep the newpm pipeline in |
|
Yes, we'll need to get in the upstream change for this change to be effective, so that CVP can recognize our negation idiom. |
|
@vchuravy could you queue that LLVM revision for whenever you're planning the next LLVM rebuild? I think it's pretty much good to go upstream just waiting for review. |
|
Important for 1.9? Or do we have a bit of time? |
|
I don't think it'll be backported to 1.9, but I do hope to merge #48066 soon and without this some packages will start seeing extra allocations. Not a big issue, but certainly we should get to it before we start seriously pkgevaling 1.10 against 1.9. |
|
Upstream merged as llvm/llvm-project@1436a92 |
vtjnash
requested review from
vchuravy
and removed request for
vchuravy and
pchintalapudi
|
Btw, the aotcompile pipeline is no longer used. For this to have an effect we need to do it in pipeline.cpp |
|
We have started the LLVM 16 build but as always it's an annoying build |
|
should we revive this now that we have llvm16? |
Not yet #51720 is not finished. |
I was investigating why LLVM was unable to remove the boundscheck that was causing the test failure in #48066 and it turned out that the check was removable by instcombine, but only if cvp ran first. However, we ran the passes in the opposite order and then loop-rotated the condition away before instcombine ran again. I think it makes sense to run cvp before instcombine to make sure that instcombine can make use of the overflow flags inferred by cvp. For the particular case in #48066, we also need https://reviews.llvm.org/D140933 (and we may need it in general, since we like to emit our conditionals as negations), but overall this seems like a better place for cvp in our pass pipeline.