[PATCH] D30081: [PPC] Eliminate more compare instructions using record-form operation

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 6 inline comments as done.
inouehrs added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1690
+    // PPC does not have a record-form SUBri.
+    if (!NonZeroImmed)
+      return false;
----------------
timshen wrote:
> The name is confusing in this scope. If Value != 0, then it literally means The immediate is not zero.
> 
> Combined with the next comment, I suggest to use a better name - `Success` is even better IMO.
> 
> Alternatively you can wrap up this scope into a function/lambda, and use the return value. That will eliminate the `Success` variable even further.
I changed the name to `Success` and made its scope local.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1762
   bool ShouldSwap = false;
-  if (Sub) {
+  if (Sub && !NonZeroImmed) {
     ShouldSwap = SrcReg2 != 0 && Sub->getOperand(1).getReg() == SrcReg2 &&
----------------
timshen wrote:
> If we use `(Sub && Value == 0)` here, the scope of NonZeroImmed can be limited to the previous small scope `else if (Value != 0) {`, right?
> 
> It seems better to have fewer states in the larger scope.
When `SrcReg2 != 0`, `Value` is not used and so not initialized. 
To avoid this problem, I initialized `Value` with zero in `analyzeCompare` above.


https://reviews.llvm.org/D30081