[PATCH] D151848: [X86, Peephole] Enable FoldImmediate for X86

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/CodeGen/PeepholeOptimizer.cpp:1357
     DenseMap<Register, MachineInstr *> &ImmDefMIs) {
   const MCInstrDesc &MCID = MI.getDesc();
+  if (MCID.getNumDefs() != 1 || !MI.getOperand(0).isReg())
----------------
davidxl wrote:
> The code looks cleaner with early return like before:
> 
> if (!MI.isMoveImmediate() && !TII->getConstValDefinedInReg(MI,Reg,ImmVal))
>     return false;
getConstValDefinedInReg needs the Reg parameter, so it can't be moved to the very beginning of the function. But I changed it to return false on the opposite condition.


================
Comment at: llvm/lib/CodeGen/PeepholeOptimizer.cpp:1358
   const MCInstrDesc &MCID = MI.getDesc();
-  if (!MI.isMoveImmediate())
-    return false;
-  if (MCID.getNumDefs() != 1)
+  if (MCID.getNumDefs() != 1 || !MI.getOperand(0).isReg())
     return false;
----------------
davidxl wrote:
> Should this be
> 
> if (MI.isMoveImmediate() && MCID.getNumDefs() != 1 
>    || !MI.isMoveImmediate() && !MI.getOperand(0).isReg())
>    return false;
My code should also be correct. The MI.isMoveImmediate() test is moved to different statement. The !MI.getOperand(0).isReg() is used to make sure the following getReg() works correctly.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4905
+
+  if (!isInt<32>(ImmVal))
+    return false;
----------------
MatzeB wrote:
> Is this correct? I haven't gone over the patch or read up on the details... But I'm worried this mixes up signed/unsigned representations. For example `int64_t{-1}` passes an `isInt<32>()` check but does not pass an `isUInt<32>()` check. Wouldn't it depend on the operation on whether we should interpret the immediate as signed or unsigned?
You are right. The actual legal type is instruction dependent.
All 32 bit immediate in a 32 bit register can be directly used in a ALU32ri instruction, so we can safely skip the checking.
For 64 bit operations only 32 bit signed integers can be used in ALU64ri instructions.


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

https://reviews.llvm.org/D151848