[PATCH] D133103: [PowerPC] Improve kill flag computation and add verification after MI peephole

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:4814
   LLVM_DEBUG(MI.dump());
   return true;
 }
----------------
amyk wrote:
> A question I had: Does this function also require a call to `recomputeLivenessFlags` after PostRA?
We have an early exit on line 4757 for post-ra.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:379
+                                   Register Dst
+                                   ) {
   DenseMap<MachineInstr *, MachineInstr *> ChangedPHIMap;
----------------
stefanp wrote:
> nit: spacing
Weird. I could have sworn I ran `clang-format` on this. I'll fix it. Thanks for noticing.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:431
+      if (MRI->isSSA())
+        addRegToUpdate(RegMBB.first.getReg());
+    }
----------------
stefanp wrote:
> We are adding `RegMBB.first` but what about `RegMBB.second` and `AccReg`? This is a new instruction so I assume that all of these registers now may have a different live range.
`RegMBB.second` is a `MachineBasicBlock` so we can't add it to the list of registers. The newly created registers won't be created with kill flags, so we don't need to recompute them and the result reg for the first PHI (`Dst`) is still defined in the exact same place, so it shouldn't need its kill flags recomputed.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:1927
   SrcMI->eraseFromParent();
+  addRegToUpdate(NewInstr->getOperand(1).getReg());
   return true;
----------------
stefanp wrote:
> There are two instructions that are erased here and I don't think all of the required registers are added to the update list. 
> I managed to get a reduced test case that I think shows this issue:
> ```
> define hidden void @testCaller() local_unnamed_addr align 2 {
> entry:
>   br label %exit
> 
> exit: ; preds = %entry
>   br label %cond.end.i.i
> 
> cond.end.i.i:                                     ; preds = %if.end116, %exit
>   %CurrentState.0566 = phi i32 [ %CurrentState.2, %if.end116 ], [ -1, %exit ]
>   %0 = load i32, ptr poison, align 8
>   br label %while.body5.i
> 
> while.cond12.preheader.i:                         ; preds = %while.body5.i
>   br i1 poison, label %if.end116, label %for.cond99.preheader
> 
> while.body5.i:                                    ; preds = %while.body5.i, %cond.end.i.i
>   %Test.012.i = phi i32 [ 0, %cond.end.i.i ], [ %dec10.i, %while.body5.i ]
>   %dec10.i = add nsw i32 %Test.012.i, -1
>   %cmp4.i = icmp slt i32 0, %dec10.i
>   br i1 %cmp4.i, label %while.body5.i, label %while.cond12.preheader.i
> 
> for.cond99.preheader:                             ; preds = %while.cond12.preheader.i
>   %1 = load ptr, ptr poison, align 8
>   %conv103 = sext i32 %0 to i64
>   %arrayidx.i426 = getelementptr inbounds i32, ptr %1, i64 %conv103
>   store i32 0, ptr %arrayidx.i426, align 4
>   store i32 %CurrentState.0566, ptr poison, align 8
>   br label %if.end116
> 
> if.end116:                                        ; preds = %for.cond99.preheader, %while.cond12.preheader.i
>   %CurrentState.2 = phi i32 [ %0, %while.cond12.preheader.i ], [ poison, %for.cond99.preheader ]
>   call fastcc void @callee()
>   br label %cond.end.i.i
> }
> declare dso_local fastcc void @callee() unnamed_addr align 2
> ```
> Compile with:
> ```
> llc --mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -debug-only=ppc-mi-peepholes bugpoint-reduced-simplified.ll
> ```
> 
> I see the following output:
> ```
> Combining pair:   %2:g8rc = EXTSW_32_64 %1:gprc
>   %12:g8rc = RLDICR killed %2:g8rc, 2, 61
> TO:   %12:g8rc = EXTSWSLI_32_64 %1:gprc, 2
> Adding register: 1 on line 1944 for re-computation of kill flags
> Deleting instruction:   %12:g8rc = RLDICR killed %2:g8rc, 2, 61
> ...
> *** Bad machine code: LiveVariables: Block should not be in AliveBlocks ***
> - function:    testCaller
> - basic block: %bb.2 while.cond12.preheader.i (0x119473888)
> Virtual register %2 is not needed live through the block.
> 
> *** Bad machine code: LiveVariables: Block should not be in AliveBlocks ***
> - function:    testCaller
> - basic block: %bb.3 while.body5.i (0x119473988)
> Virtual register %2 is not needed live through the block.
> LLVM ERROR: Found 2 machine code errors.
> ```
Ah, I see. Good catch. I think I'll just add the result register of `SrcMI` to the set and leave the `SrcMI` alone (DCE is run immediately after this pass to remove dead instructions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133103