[PATCH] D78657: [PowerPC] Fix the liveins for ppc-expand-isel pass

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang marked 4 inline comments as done.
ZhangKang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCExpandISEL.cpp:390
-    // in MBB should be treated as liveins of NewSuccessor.
-    // Note: Cannot use stepBackward instead since we are using the Reg
-    // liveness state at the end of MBB (liveOut of MBB) as the liveIn for
----------------
lkail wrote:
> ZhangKang wrote:
> > lkail wrote:
> > > I notice `computeAndAddLiveIns` will call `stepBackward`, this comment says it will lead to cyclic dependence. Is it still the case for current code?
> > 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.
> 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.
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++;
```


================
Comment at: llvm/test/CodeGen/PowerPC/expand-isel-liveness.mir:50
   ; CHECK:   BLR8 implicit $lr8, implicit $rm, implicit killed $x3, implicit killed $x4, implicit killed $x5
 ...
----------------
lkail wrote:
> Could you provide test case that `ISEL` is the last instruction of a MBB?
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78657/new/

https://reviews.llvm.org/D78657