Page MenuHomePhabricator

[PowerPC] Fix the killed flag in mi-peephole pass
AbandonedPublic

Authored by qiucf on Jul 10 2020, 9:28 AM.

Details

Reviewers
nemanjai
jsji
steven.zhang
ZhangKang
Group Reviewers
Restricted Project
Summary

When doing the peephole, we move the killed flag from srcMI to destMI without do more check,
this is not right.

For below example, %3 is in a loop, %2 is out of the loop.

bb1:
   %2:g8rc = RLDICL killed %1, 0, 32
bb2:
   %3:g8rc = RLDICR %2, 2, 61    // can fold to %3:g8rc = RLDIC %1, 2, 30, %1 shouldn't has the kill flag.
   ...
   BCC 4, $cr0, %bb.2

This patch uses the conservative method to set the kill flag.
It can fix 62 lnt erros and 22 lit errors.

Diff Detail

Unit TestsFailed

TimeTest
50 mslinux > LLVM.CodeGen/PowerPC::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=powerpc64le--linux-gnu -start-before ppc-mi-peepholes /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir -o - -verify-machineinstrs | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/jump-tables-collapse-rotate-remove-SrcMI.mir
460 mslinux > MemorySanitizer-X86_64.MemorySanitizer-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/__strxfrm_l.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/__strxfrm_l.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/__strxfrm_l.cpp.tmp
510 mslinux > MemorySanitizer-X86_64.MemorySanitizer-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp
250 mslinux > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/__strxfrm_l.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/__strxfrm_l.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/__strxfrm_l.cpp.tmp
260 mslinux > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp
View Full Test Results (16 Failed)

Event Timeline

ZhangKang created this revision.Jul 10 2020, 9:28 AM

Fix the error case.

shchenz added inline comments.Jul 16 2020, 7:21 PM
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1562

Should we also consider SrcMI->getOperand(0) or legacy MI.getOperand(1) at least when MI and and SrcMI are in same BB?

We have a API fixupIsDeadOrKill for kill/dead flag fixup.

ZhangKang marked an inline comment as done.Jul 16 2020, 9:05 PM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1562

The SrcMI will be removed if it's necessary in the line 1571.

The fixupIsDeadOrkill is used for non-ssa, but the pass is a SSA pass. So, we can't using it.

shchenz added inline comments.Jul 16 2020, 9:30 PM
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
1562

The function fixupIsDeadOrkill should be extended to non-ssa in patch https://reviews.llvm.org/D81723.

If you want to completely fix up the kill/dead flag, there are some situations need to be considered, like:
1:

%2:g8rc = RLDICL %1, 0, 32
STORE killed %1
%3:g8rc = RLDICR %2, 2, 61   ;; killed flag from above STORE should be sunk down here

2:

 %2:g8rc = RLDICL killed %1, 0, 32
STORE  %2
 %3:g8rc = RLDICR killed %2, 2, 61 ;;  killed flag for %2 should be hoisted up to above STORE

fixupIsDeadOrkill can do this for you.

ZhangKang updated this revision to Diff 278954.Jul 17 2020, 6:26 PM
ZhangKang marked 2 inline comments as done.

Call IsKilledFor() to fix the flag.

ZhangKang updated this revision to Diff 279232.Jul 20 2020, 7:05 AM

Remove the dump code.

Please also add test case for the new scenarios

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

I think you mean SrcMI->getOperand(1)?

ZhangKang updated this revision to Diff 283127.Aug 4 2020, 10:17 PM

Add a new func setKillAfterFolding.

ZhangKang edited the summary of this revision. (Show Details)Aug 4 2020, 10:27 PM

It seems like we keep adding more and more complexity here to handle these flags. Is there not a pass we can just run that will simply re-compute this stuff (at least for the use of this while we're in SSA)? What does LiveVariables do? We certainly end up running DCE after the MI peephole to get rid of dead instructions after the transformation. It seems like we should be able to do the same thing for killed/dead flags as well.

qiucf commandeered this revision.Sep 9 2020, 11:33 PM
qiucf added a reviewer: ZhangKang.
qiucf abandoned this revision.Sep 29 2020, 10:41 PM

Since the motivation case is fixed by rG69289cc1 and this patch is a little bit aggressive (causing more crashes), close it.