Page MenuHomePhabricator

[PowerPC] Improve kill flag computation and add verification after MI peephole
Needs ReviewPublic

Authored by nemanjai on Sep 1 2022, 4:39 AM.

Details

Reviewers
qiongsiwu
Group Reviewers
Restricted Project
Summary

The MI Peephole pass has grown to include a large number of transformations over the years. Many of the transformations require re-computation of kill flags but don't do a good job of re-computing them. This causes us to have very common failures when the compiler is built with expensive checks. Over time, we added and augmented a function that is supposed to go and fix up kill flags after each transformation but we keep missing cases.
This patch does the following:

  • Removes the function to re-compute kill flags
  • Adds LiveVariables to compute and maintain kill flags while transforming code
  • Adds re-computation of kill flags for the post-RA peepholes for each block that contains a transformed instruction
  • Improves some of the LLVM_DEBUG messages and adds missing ones
  • Does a bit of general clean-up in the pass

Diff Detail

Unit TestsFailed

TimeTest
30 msx64 debian > LLVM.CodeGen/PowerPC::sext_elimination.mir
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -run-pass ppc-mi-peepholes -ppc-eliminate-signext -ppc-eliminate-zeroext -verify-machineinstrs -o - /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/sext_elimination.mir | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/PowerPC/sext_elimination.mir

Event Timeline

nemanjai created this revision.Sep 1 2022, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 4:39 AM
nemanjai requested review of this revision.Sep 1 2022, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 4:39 AM
lkail added a subscriber: lkail.Sep 1 2022, 5:50 AM
amyk added a subscriber: amyk.Sep 21 2022, 4:28 PM

Is it just me or is this patch not clean? It appears there is a LIT failure in CodeGen/PowerPC/sext_elimination.mir and I am getting some test-suite build failures.

Is it just me or is this patch not clean? It appears there is a LIT failure in CodeGen/PowerPC/sext_elimination.mir and I am getting some test-suite build failures.

Interesting. I was not seeing failures, but something may have changed between my revision and yours. I'll have a look.

amyk added a comment.Wed, Nov 2, 9:56 AM

Some initial general comments for this patch:

  • The description doesn't exactly clarify how the flags are recomputed for pre/post-RA. Can the description be clarified a bit more?
  • The improvement of the LLVM_DEBUG messages and general code clean up could be a separate patch. Can this stuff be separated out?
amyk added a comment.Thu, Nov 17, 7:19 PM

Just had a couple comments I wanted to submit prior to the rebase of this patch.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3632

Nit: unrelated whitespace change.

3651

Isn't this an unrelated change, as well?

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
468

We're accessing the register both in the cases when we're removing it from RegsToUpdate and when we're re-running LiveVariables.
It might make sense to pull out MO.isReg() into a separate variable.

1817

Nit: unrelated whitespace change.

amyk added a comment.Wed, Nov 23, 5:43 PM

A few more submitted comments that I realized I had left on the review.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
3638

We should clarify that for MI, we're actually adding both the defs and the uses.

3642

It would be good to have a more descriptive name for UpdateRegs. Perhaps RegsToUpdate.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
439

Since you're adding a significant amount of code to a deeply nested block, consider flipping this condition to an early exit to reduce the nesting.