[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:1391
+      // FoldImmediate can delete ImmDefMI if MI was its only user. If ImmDefMI
+      // is not deleted, and we happenly get a same MI, we can delete MI and
+      // replace its users.
----------------
craig.topper wrote:
> happenly?
The case I encountered is
```
     %1 = MOV32ri   4
     %2 = COPY %1
      // other uses of %1
     ...
```
After FoldImmediate it becomes
```
    %1 = MOV32ri 4
    %2 = MOV32ri 4
```
Becuase %1 has multiple uses, so it can't be deleted by FoldImmediate. These two instructions are identical now, so we can replace the uses of %2 by %1, and delete the definition of %2.


================
Comment at: llvm/test/CodeGen/X86/popcnt.ll:619
+; X86-NEXT:    movl %eax, %ecx
+; X86-NEXT:    andl $858993459, %ecx # imm = 0x33333333
 ; X86-NEXT:    shrl $2, %eax
----------------
craig.topper wrote:
> This defeats the code size optimization in SelectionDAG.  See X86InstrInfo.td
> 
> ```
> // If we have multiple users of an immediate, it's much smaller to reuse         
> // the register, rather than encode the immediate in every instruction.          
> // This has the risk of increasing register pressure from stretched live         
> // ranges, however, the immediates should be trivial to rematerialize by         
> // the RA in the event of high register pressure.                                
> // TODO : This is currently enabled for stores and binary ops. There are more    
> // cases for which this can be enabled, though this catches the bulk of the      
> // issues.                                                                       
> // TODO2 : This should really also be enabled under O2, but there's currently    
> // an issue with RA where we don't pull the constants into their users           
> // when we rematerialize them. I'll follow-up on enabling O2 after we fix that   
> // issue.                                                                        
> // TODO3 : This is currently limited to single basic blocks (DAG creation        
> // pulls block immediates to the top and merges them if necessary).              
> // Eventually, it would be nice to allow ConstantHoisting to merge constants     
> // globally for potentially added savings.                                       
> //                                                                               
> def imm_su : PatLeaf<(imm), [{                                                   
>     return !shouldAvoidImmediateInstFormsForSize(N);                             
> }]>;
> ```
I need to take a look at this. 
thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151848