[PassBuilder] Add RelLookupTableConverterPass to LTO

[gulfemsavrun]

[PassBuilder] Add RelLookupTableConverterPass to LTO

This patch adds RelLookupTableConverterPass into the LTO
post-link optimization pass pipeline. This optimization
converts lookup tables to relative lookup tables to make
them PIC-friendly, which is already included in the non-LTO
pass pipeline. This patch adds this optimization to the
post-link optimization pipeline to discover more
opportunities in the LTO context.

[ilovepi]

Shouldn't this be much later in the pipeline? I'd guess after MergeFunctions, but before CGProfile is probably a better spot(

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Line 2123 in 77c23fd

) for high opt levels. You'll probably want to add it before the returns for lower opt levels too.

I suppose it could arguably be added to Late EP callbacks, as that would also apply to other optimization levels, w/o needing to specifically add the pass everywhere.

CC: @aeubanks @nikic @efriedma-quic

[gulfemsavrun]

I moved the pass later in the LTO pipeline. The early return for other optimization levels are for -O0 and -O1, and I think it is reasonable to add this pass when built with -O2 and higher.

[ilovepi]

Well, I was thinking that wanting PIC friendly lookup tables isn't necessarily something tied to the optimization level. For instance, I'd suppose if you care about PIC friendliness, you'd want it on even at low opt levels. That's also something easy to revisit, though, so its probably fine to start here and revisit as needed.

[gulfemsavrun]

Let's revisit this because I'm not sure whether this optimization should be included in O0 or O1 because those opt levels are limited in terms of the passes that they include. This is not a costly optimization, but let's follow up on this.

[ilovepi]

Do we need to update any tests? I seem to recall several pipeline tests that checked the order of passes. I'm not sure if we have any of those for LTO, though, as I don't see them in the test dir.

Assuming CI is happy w/ this, LGTM, but lets confirm w/ @aeubanks or @nikic before landing. Pass ordering has a tendency to have unintended consequences, so I'd feel better if one of them double checked me on that, since we're so close to the branch point.

[ilovepi]

Oh, does moving a pass in the pipeline need a release note?

[nikic]

I do believe you need to adjust pipeline tests, but the change itself LGTM.

I'd also drop this TODO:

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Lines 1597 to 1599 in f569585

	// TODO: Relative look table converter pass caused an issue when full lto is
	// enabled. See https://reviews.llvm.org/D94355 for more details.
	// Until the issue fixed, disable this pass during pre-linking phase.

While running the pass in that position ran into an issue, the pass should not be run pre-link in any case, even if that issue is resolved.

[aeubanks]

please add some background/motivation in the commit description, and explicitly mention "RelLookupTableConverterPass" in the commit title

[gulfemsavrun]

I do believe you need to adjust pipeline tests, but the change itself LGTM.

I'd also drop this TODO:

llvm-project/llvm/lib/Passes/PassBuilderPipelines.cpp

Lines 1597 to 1599 in f569585

	// TODO: Relative look table converter pass caused an issue when full lto is
	// enabled. See https://reviews.llvm.org/D94355 for more details.
	// Until the issue fixed, disable this pass during pre-linking phase.

While running the pass in that position ran into an issue, the pass should not be run pre-link in any case, even if that issue is resolved.

I removed that todo, and adjusted pipeline tests.

[gulfemsavrun]

please add some background/motivation in the commit description, and explicitly mention "RelLookupTableConverterPass" in the commit title

Done.

[gulfemsavrun]

Oh, does moving a pass in the pipeline need a release note?

Which release note should this go in? Clang maybe?
https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst

[aeubanks]

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes

I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

[ilovepi]

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes

I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

Agreed that's a best practice, but I'm pretty sure if you're merging through github UI you can also edit the commit message and title before landing.

[gulfemsavrun]

llvm/docs/ReleaseNotes.md is the place to add release notes for pipeline changes
I think for the final commit to have the commit message/description, the PR's title and first comment must contain it, at least for squash and merge like LLVM does

Agreed that's a best practice, but I'm pretty sure if you're merging through github UI you can also edit the commit message and title before landing.

Oh, I did not realize that commit message is not updated after I updated in the branch itself. Fixed it now!