This is an archive of the discontinued LLVM Phabricator instance.

[LSR][TTI][RISCV] Add isAllowTerminatingConditionFoldingAfterLSR into TTI and enable it for RISC-V
AbandonedPublic

Authored by reames on Sep 29 2022, 9:39 AM.

Details

Summary

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.

Diff Detail

Event Timeline

eopXD created this revision.Sep 29 2022, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 9:39 AM
eopXD requested review of this revision.Sep 29 2022, 9:39 AM
eopXD added a comment.EditedSep 29 2022, 9:41 AM

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.

JojoR added a subscriber: JojoR.Oct 11 2022, 1:53 AM

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 :

eopXD added a comment.Oct 11 2022, 4:15 AM

@JojoR Thanks for the report. I was wondering that whether the assertion would hit. Will add a patch to fix this.

JojoR added a comment.Oct 13 2022, 2:09 AM

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 ?

eopXD added a comment.Oct 13 2022, 2:14 AM

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.

JojoR added a comment.Oct 13 2022, 2:26 AM

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.

Got it, thanks :)

JojoR added a comment.Oct 13 2022, 2:49 AM

@eopXD I saw the patch D132443 only support eq/neq condterm, and I want to add gt/lt,
any suggestion for me ? or you have plan to implement that ?

eopXD added a comment.Oct 13 2022, 4:28 AM

@eopXD I saw the patch D132443 only support eq/neq condterm, and I want to add gt/lt,
any suggestion for me ? or you have plan to implement that ?

I will submit proceeding patches for more support, thank you.

eopXD added a comment.Oct 20 2022, 8:30 PM

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 :

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.

eopXD updated this revision to Diff 469899.Oct 22 2022, 7:52 AM

Rebase to latest main

I think we should start discussion again, because this pass has changed.

@eopXD
@JojoR

  1. 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

  1. 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?

eopXD added a subscriber: fhahn.Mar 21 2023, 1:21 AM

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.)

I will note that I am generally skeptical of this being enabled on a per-target basis.

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.

eopXD updated this revision to Diff 525965.May 26 2023, 12:17 AM

Rebase to latest main.

evandro removed a subscriber: evandro.May 26 2023, 10:32 AM
eopXD updated this revision to Diff 526313.May 27 2023, 10:11 PM

Resolve other test case failures.

reames added a comment.Jul 7 2023, 8:23 AM

Reverse ping. Any progress on that testing summary?

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

reames commandeered this revision.Wed, Nov 29, 12:14 PM
reames edited reviewers, added: eopXD; removed: reames.
reames abandoned this revision.Wed, Nov 29, 12:14 PM

A reworked version of this has been landed in e947f953370abe8ffc8713b8f3250a3ec39599fe.