Page MenuHomePhabricator

[PowerPC] Fix the liveins for ppc-expand-isel pass
ClosedPublic

Authored by ZhangKang on Apr 22 2020, 11:11 AM.

Details

Summary

In the ppc-expand-isel pass, we use stepForward() to update the liveness, this function is not recommended, because it need the accurate kill info.

In fact, we have the function computeAndAddLiveIns(), it will use stepBackward() to update the liveness. It's the recommended method to update the liveins.

I have use the option machine-verifier-strict-livein provided in https://reviews.llvm.org/D78586/ to test this patch.
In the patch D78586, there are 57 PowerPC cases failed, after using this patch, 54 cases can pass, and the left 3 cases should be the cases issue.

Diff Detail

Event Timeline

ZhangKang created this revision.Apr 22 2020, 11:11 AM
efriedma added inline comments.Apr 22 2020, 12:25 PM
llvm/lib/Target/PowerPC/PPCExpandISEL.cpp
454

Is the clearLiveIns() call necessary? I would have thought that TrueBlock has no liveins set, since you just created it.

lkail added a subscriber: lkail.Apr 22 2020, 7:52 PM
lkail added inline comments.
llvm/lib/Target/PowerPC/PPCExpandISEL.cpp
390

I notice computeAndAddLiveIns will call stepBackward, this comment says it will lead to cyclic dependence. Is it still the case for current code?

ZhangKang marked 3 inline comments as done.Apr 22 2020, 8:30 PM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCExpandISEL.cpp
390

If the livenins of successors are right, we can using stepBackward method to calculate the livenins for this MachineBasicBlock.
Here, the successors of MBB are equal the to the successors of NewSuccessor, and the liveins of successors are right for MBB. So we can use stepBackward method to calculate the liveness for NewSuccessor.

454

If there are more than one ISEL for BIL, we will get assert error the TrueBlock has the liveins for last ISEL instruction.
For example:

bb.0.entry:
    liveins: $x4, $x5, $x6, $x7

    renamable $cr0 = CMPWI renamable $r7, 0
    renamable $r3 = ISEL killed renamable $r7, killed renamable $r4, renamable $cr0gt, implicit $x4, implicit $x7
    renamable $r4 = ISEL killed renamable $r5, killed renamable $r6, killed renamable $cr0gt, implicit $cr0, implicit $x6, implicit $x5
    renamable $r3 = nsw ADD4 killed renamable $r3, killed renamable $r4
    renamable $x3 = EXTSW_32_64 killed renamable $r3
    BLR8 implicit $lr8, implicit $rm, implicit killed $x3

We will get:

bb.0.entry:
  successors: %bb.2(0x40000000), %bb.1(0x40000000)
  liveins: $x4, $x5, $x6, $x7

  renamable $cr0 = CMPWI renamable $r7, 0
  BC killed renamable $cr0gt, %bb.2

bb.1.entry:
  successors: %bb.3(0x80000000)
  liveins: $r6, $r4

  renamable $r3 = ORI killed renamable $r4, 0
  renamable $r4 = ORI killed renamable $r6, 0
  B %bb.3

bb.2.entry:
  successors: %bb.3(0x80000000)
  liveins: $r5, $r7

  renamable $r3 = ADDI killed renamable $r7, 0
  renamable $r4 = ADDI killed renamable $r5, 0

bb.3.entry:
  liveins: $r3, $r4

  renamable $r3 = nsw ADD4 killed renamable $r3, killed renamable $r4
  renamable $x3 = EXTSW_32_64 killed renamable $r3
  BLR8 implicit $lr8, implicit $rm, implicit killed $x3

But your comment reminder me that I should calculate the liveins out of the for loop. I will update the patch.

ZhangKang marked an inline comment as done.Apr 22 2020, 8:35 PM
ZhangKang updated this revision to Diff 259468.Apr 22 2020, 8:49 PM

Move update liveins out of for loop.

ZhangKang edited the summary of this revision. (Show Details)Apr 22 2020, 8:56 PM
This comment was removed by lkail.
lkail added inline comments.Apr 23 2020, 2:39 AM
llvm/lib/Target/PowerPC/PPCExpandISEL.cpp
390

Ok, I get it. The origin implementation

// Copy the original liveIns of MBB to NewSuccessor.

has been problematic. We might have some registers killed before the isel instruction.

llvm/test/CodeGen/PowerPC/expand-isel-liveness.mir
50–80

Could you provide test case that ISEL is the last instruction of a MBB?

ZhangKang marked 4 inline comments as done.Apr 26 2020, 1:34 AM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCExpandISEL.cpp
390

Only for the special case 1, all registers used by ISEL are the same one, we should care about the killed/dead flag. I think it should be fixed in another patch.

208       // Special case 1, all registers used by ISEL are the same one.
209       // The non-redundant isel 0, 0, 0, N would not satisfy these conditions
210       // as it would be ISEL %R0, %ZERO, %R0, %CRN.
211       if (useSameRegister(Dest, TrueValue) &&
212           useSameRegister(Dest, FalseValue)) {
213         LLVM_DEBUG(dbgs() << "Remove redundant ISEL instruction: " << **I
214                           << "\n");
215         // FIXME: if the CR field used has no other uses, we could eliminate the
216         // instruction that defines it. This would have to be done manually
217         // since this pass runs too late to run DCE after it.
218         NumRemoved++;
219         (*I)->eraseFromParent();
220         I++;
llvm/test/CodeGen/PowerPC/expand-isel-liveness.mir
50–80

Done.

ZhangKang updated this revision to Diff 260152.Apr 26 2020, 1:38 AM
ZhangKang marked 2 inline comments as done.

Add a new case whose last MI is ISEL.

efriedma accepted this revision.Apr 27 2020, 2:23 PM

LGTM (but please wait to see if @lkail has further comments)

This revision is now accepted and ready to land.Apr 27 2020, 2:23 PM
lkail accepted this revision.Apr 27 2020, 6:09 PM

LGTM. Thanks for fixing it.

This revision was automatically updated to reflect the committed changes.