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.
Details
- Reviewers
craig.topper mcberg2021 rogfer01 asb eopXD
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I ran through lit test under llvm/test/CodeGen/RISCV before submitting this patch, these two affected test case shows the benefit of the transformation.
There is a error report as below if i apply this patch on branch main 8d4ebd1a7c9e1f47a4a610aeb41d1613f822ee20
error: Terminating value is not safe to expand, need to add it to predicate.
my compiler option is "-march=rv64gc -O2"
my test case is :
@JojoR Thanks for the report. I was wondering that whether the assertion would hit. Will add a patch to fix this.
I have a question about patch D132443, why do not you put optimization in pass "iv-users" ? I think it belongs to IV analyzation.
because that the pass "iv-users" do analyzation only without any transformation ?
IVUser is an analysis pass, and the LSR pass uses it before doing strength reduction.
This transformation is beneficial after LSR, not before it.
Is it possible for you to have a reduced test case on this? It would help to land the revision to fix this. Thank you.
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.
lsr-term-fold is still wildly unsound. I have patches out to address this, but we should wait until that is done discussing enabling it (by default or by target).
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?
llvm/include/llvm/Analysis/TargetTransformInfo.h | ||
---|---|---|
691 | Please change to: shouldFoldTerminatingConditionAfterLSR |
A reworked version of this has been landed in e947f953370abe8ffc8713b8f3250a3ec39599fe.
Please change to: shouldFoldTerminatingConditionAfterLSR