[PATCH] D89855: [PowerPC] Extend folding RLWINM + RLWINM to post-RA.

[EsmeYi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3408
     MI.getOperand(2).setImm(NewSH);
     // If SrcMI mask is full, no need to update MBMI and MEMI.
     if (!SrcMaskFull) {
----------------
nemanjai wrote:
> No need, but is it incorrect to update them? Namely, won't the `NewMB/NewME` be the same as `MBMI/MEMI` respectively? If so, I think it is preferable to avoid the condition and leave it to the reader to figure out.
It's incorrect to update MBMI/MEMI with NewMB/NewME if the SrcMI mask is full, since isRunOfOnes(FinalMask) may not hold true in such scenario. Well might  it be more clear to replace `no need` with `don't`?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3414
     MI.getOperand(1).setReg(SrcMI->getOperand(1).getReg());
     if (SrcMI->getOperand(1).isKill()) {
       MI.getOperand(1).setIsKill(true);
----------------
nemanjai wrote:
> In general, I find that manually messing with kill flags when changing the instructions to be an error-prone thing that should be avoided if possible? Is it possible to re-run the pass that computes kill flags after the peephole?
> 
> For example I think that here, if `MI.getOperand(1).isKill()` before the change, we will not update the kill flag on the previous use of that register so it will be considered live after `MI` whereas before it wasn't. This is of course not semantically incorrect, but may hinder some optimizations.
Good catch! However I have no idea which pass can solve the problem. It seems that LiveInternals Pass will computes kill flags, but it can't handle the case you mentioned. Do you have any idea about this?


================
Comment at: llvm/test/CodeGen/PowerPC/vsx_builtins.ll:138
   %0 = tail call i32 @llvm.ppc.vsx.xvtdivdp(<2 x double> %a, <2 x double> %b)
   %1 = lshr i32 %0, 4
   %.lobit = and i32 %1, 1
----------------
nemanjai wrote:
> It is not really related to this patch, but what is this test case supposed to do? Why would we produce a 4-bit field in a CR, move it to a GPR and then right shift by 4 (clearly shifting out all the bits)?
Yes. you're right. lol... This test case was designed to show the folding patter, `rlwinm + rlwinm -> li 0` in post-ra.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89855