Previously in D132443, the transformation was added and guarded by an option.
This commit attempts to create an TTI and enable it for the RISC-V backend.
I think we should start discussion again, because this pass has changed.
- Implemented : https://reviews.llvm.org/D145929
I saw the patch D132443 only support eq/neq condterm, and I want to add gt/lt,
any suggestion for me
- Implemented : https://reviews.llvm.org/D136415
There is a error report as below if i apply this patch on branch main
error: Terminating value is not safe to expand, need to add it to predicate.
So, may we should rebase this MR and apply changes?
I agree with @fhahn that such hook to a middle-end optimization like LSR is not a great approach. I have generally improving LSR term-fold and default enabling it as an item to my queue. I have the RVV intrinsics to be dealt with first but this will be the first thing I'll revisit once I am done there.
If there is no further comments I will drop this patch later this week.
Ok, at this point all the known soundness problems are fixed in tree. I was simply fixing ones obvious on inspection, and have not done any testing of this mechanism beyond the LIT tests themselves.
Before we move to discussing whether lsr-term-fold makes sense to enable by default, and on what basis, I think we need to have a discussion about validation and testing. Whoever is going to drive this patch forward needs to describe their testing, and validate that after all the bug fixes this still triggers enough to be worthwhile.
I will note that I am generally skeptical of this being enabled on a per-target basis. There needs to be a compelling argument as why this shouldn't be enabled more broadly. One of the major advantages of enabling this globally is increasing testing, and thus smoking out bugs more quickly. Given existing problems with testing, I think that advantage is one I'm very reluctant to give up. I also think that all targets should benefit from the transform as current framed, so I don't see any reason not to enable it. (I'm open to counter arguments here; they just need to be made.)
To circle back around here, I spent some time looking at the impact of this on other targets. In short, this heuristic really only makes sense on RISCV at the moment. Given this, my prior objection to a transform specific target hook no longer applies.
I do want to see some discussion of the testing this patch has been put through before we move forward with enabling it though. I'd encourage a rebase and a summary comment describing any testing which has been done.
Reverse ping. I applied this locally, and collected dynamic instruction counts for SPEC on a rv64gcv config. The results didn't reveal any surprises or correctness issues. Dynamic instruction count was very mildly improved overall (0.42% geomean improvement), with no regression larger than 0.25%.
I'd like to see this landed. Can you rebase for a final LGTM?
Please change to: shouldFoldTerminatingConditionAfterLSR