[PATCH] D40554: [PowerPC] Fix bugs in sign-/zero-extension elimination

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5092
 
 // This function returns true if the machine instruction
 // always outputs a value by sign-extending a 32 bit value,
----------------
amyk wrote:
> Question: should we update this to say if the `register` and not `machine instruction` since it is changed to `Reg` for the parameter? 
> Or should it stay as machine instruction since you're checking for the different `MachineInstr` opcodes in this function?
> The same question I have applies to `definedByZeroExtendingOp()`.
I think it should probably say "machine instruction that defines this register"  because that is what we are really interested in. We want to make sure that this register is already sign extended by the instruction that defines it. I'm going to re-write the comment to make it clear.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:5314
+    auto SrcExt = isSignOrZeroExtended(SrcReg, BinOpDepth, MRI);
+    return std::pair<bool, bool>(SrcExt.first | IsSExt, SrcExt.second | IsZExt);
   }
----------------
amyk wrote:
> If the change to logical or still applies, we should probably update these accordingly.
Sure. I can change it to logical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D40554