This is an archive of the discontinued LLVM Phabricator instance.

[CGP] Remove dead PHI nodes before elimination of mostly empty blocks
ClosedPublic

Authored by skatkov on Aug 22 2023, 3:44 AM.

Details

Summary

Before elimination of mostly empty block it makes sense to remove dead PHI nodes.
It open more opportunity for elimination plus eliminates dead code itself.

It appeared that change results in failing many unit tests and some of them I've updated
and for another one I disable this optimization.
The pattern I observed in the tests is that there is a infinite loop without side effects.
As a result after elimination of dead phi node all other related instruction are also removed and
tests stops to check what it is expected.
I guess it is due to test is obtained by bugpoint.

I've added people related to these tests to check whether they are ok with update of the tests.

Diff Detail

Event Timeline

skatkov created this revision.Aug 22 2023, 3:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 3:44 AM
skatkov requested review of this revision.Aug 22 2023, 3:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 3:44 AM

AArch64 changes look fine; it looks like atomic lowering is generating weird code that gets simplified here. (It was mostly getting simplified anyway, but after register allocation, so the register couldn't be eliminated.)

AArch64 changes look fine; it looks like atomic lowering is generating weird code that gets simplified here. (It was mostly getting simplified anyway, but after register allocation, so the register couldn't be eliminated.)

Thank you, Eli for checking AARch64 changes!

Hi Eli, as no other feedbacks for a while, may I ask you to review the patch (it looks like it is a simple dead code elimination) and approve if there is no objections?

I'll wait say for Tue to get any possible another feedbacks and land it if no any objections.

I guess if someone will want to update tests it can be done as follow-up.
What do you think?

The change seems to be an improvement overall.

I don't see any reason anyone would object to the test changes that just disable the transform; that should preserve the original intent of the tests,

A few of the x86 test changes seem a bit tricky to review, particularly llvm/test/CodeGen/X86/tail-opts.ll

llvm/lib/CodeGen/CodeGenPrepare.cpp
887

Maybe this could use a slightly better comment, like "delete phi nodes that could block deleting other empty blocks).

skatkov updated this revision to Diff 553835.Aug 27 2023, 9:53 PM

Thank you, Eli.

I've updated comment and revert back changes for tail-opts.ll

Please take a look.

efriedma accepted this revision.Aug 28 2023, 12:44 PM

Please run some quick benchmarking to make sure this doesn't significantly slow down compile-time. (I don't think it should, but RecursivelyDeleteDeadPHINode is a recursive algorithm, so it isn't entirely obvious.)

Otherwise LGTM

This revision is now accepted and ready to land.Aug 28 2023, 12:44 PM
skatkov added a subscriber: nikic.

Thank you, Eli.

Let me do this in the following way, as I did not run compile time benchmarking for LLVM before, I'll land this patch and will monitor http://llvm-compile-time-tracker.com/index.php, if there will be some noticeable regression from my patch I will revert the patch and will think about what to do later.

I've added @nikic to reviewers as he monitored compile time as I know.