[PATCH] D125564: Teach PeepholeOpt to eliminate redundant copy from constant physreg (e.g VLENB on RISCV)

Mon May 16 14:26:18 PDT 2022

craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

In D125564#3517184 <https://reviews.llvm.org/D125564#3517184>, @reames wrote:

> In D125564#3513834 <https://reviews.llvm.org/D125564#3513834>, @craig.topper wrote:
>
>> This looks ok to me, but I'd like to understand some things about the larger roadmap here.
>>
>> If SelectionDAG CSEd the read_register this test case wouldn't need this patch. The peephole pass code here seems to be restricted to a single basic block, same as SelectionDAG. read_register isn't CSEd in SelectionDAG because read_register and read_volatile_register use the same code and have a chain dependency.
>>
>> The main places we create PseudoVLENB are from ISD::VSCALE lowering and frame lowering.
>>
>> For ISD::VSCALE lowering we go through RISCVISD::READ_VLENB. Do you plan to replace with RISCVISD::READ_VLENB with ISD::READ_REGISTER? Or make RISCVISD::READ_VLENB isel to a COPY? Or maybe use ISD::CopyFromReg? RISCVISD::READ_VLENB and ISD::CopyFromReg should CSE as long as the chain input is EntryNode and there is no Glue.
>>
>> For the frame lowering case, that happens after regalloc so this patch doesn't help with that.
>>
>> Do you have other cases that this patch helps that wouldn't already be covered by SelectionDAG CSE?
>
> Ok, two level response here.
>
> First, this is a generic improvement to target independent code.  We have the notion of constant physical registers.  Regardless of whether there are practice benefits on the particular example constant register, having the generic infrastructure handle more cases (at near zero cost) seems worthwhile on it's own.

Yeah I'm not opposed to this. I guess part of my concern was that this becomes untested if we fix SelectionDAG CSE. Maybe we should have a directed MIR test?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125564