[PATCH] D142084: [RFC][X86][MemFold] Upgrade the mechanism of auto-generated Memory Folding Table

Bing Yu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 16 00:34:35 PDT 2023


yubing added inline comments.


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:442
+    Result.CannotUnfold = BaseRegInst->isMoveReg ? true : Result.CannotUnfold;
+  } else if (RegInstr->isMoveReg && Result.IsStore)
     Result.CannotUnfold = true;
----------------
craig.topper wrote:
> craig.topper wrote:
> > yubing wrote:
> > > craig.topper wrote:
> > > > yubing wrote:
> > > > > yubing wrote:
> > > > > > skan wrote:
> > > > > > > Why do we need `Result.IsStore` here?
> > > > > > load such as VMOVAPDZ128rm will never be unfolded.
> > > > > > if VMOVAPDZ128rm 's memoperand is invariant, then VMOVAPDZ128rm  will be hoisted directly and won't be unfolded into rm+rr
> > > > > > if VMOVAPDZ128rm 's memoperand is not invariant, we stop doing unfolding since compiler find it is a simple load.
> > > > > > ```
> > > > > >   // First check whether we should hoist this instruction.
> > > > > >   if (!IsLoopInvariantInst(*MI) || !IsProfitableToHoist(*MI)) {
> > > > > >     // If not, try unfolding a hoistable load.
> > > > > >     MI = ExtractHoistableLoad(MI);
> > > > > >     if (!MI) return false;
> > > > > >   }
> > > > > > ```
> > > > > > ```
> > > > > > MachineInstr *MachineLICMBase::ExtractHoistableLoad(MachineInstr *MI) {
> > > > > >   // Don't unfold simple loads.
> > > > > >   if (MI->canFoldAsLoad())
> > > > > >     return nullptr;
> > > > > > 
> > > > > > ```
> > > > > > @craig.topper , does line429~439 make sense to you as well?
> > > > > @craig.topper i think my analysis of VMOVAPDZ128rm is only for LICM. but for other passes in codegen, is it possible for VMOVAPDZ128rm to be unfolded?
> > > > The other 2 places I know that unfold are TwoAddressInstructionPass and SelectionDAG->MachineIR
> > > > 
> > > > TwoAddressInstuctionPass should only happen for instructions with tied source and dest. That doesn't apply to moveReg.
> > > > 
> > > > I think SelectionDAG->MachineIR case happens if we need to duplicate an instruction that has an EFLAGS def. If the EFLAGS are used by two instructions and EFLAGS is clobbered in between them. We need to duplicate the flag producing instruction to satisfy the second user. But we can't duplicate the folded load so we have to unfold. That doesn't apply to moveReg.
> > > i saw two kinds of unfoldMemoryOperand, one of which is for MIR and another is for DAG. Our folding table can't affect unfoldMemoryOperand for DAG, right?
> > > and unfoldMemoryOperand for MIR only happens in LICM, TwoAddress, X86CMOVConversion, X86KCFI, and X86SpeculativeLoadHardening.
> > > i checked VMOVAPDZ128rm  won't be handled in those Pasess. so we don't worry if we need to set TB_NO_REVERSE for VMOVAPDZ128rm.
> > I think the DAG unfoldMemoryOperand uses the same table.
> The DAG unfold occurs after the isel portion of SelectionDAG. So it's MachineOpcodes
oh, you're right. unfoldMemoryOperand is trying to find the MIR according to DAGNode's Machinecode, and then lookupUnfoldTable
```
bool
X86InstrInfo::unfoldMemoryOperand(SelectionDAG &DAG, SDNode *N,
                                  SmallVectorImpl<SDNode*> &NewNodes) const {
  if (!N->isMachineOpcode())
    return false;

  const X86MemoryFoldTableEntry *I = lookupUnfoldTable(N->getMachineOpcode());
```
@craig.topper , but where can i find the code of DAG unfold which occurs after the isel portion of SelectionDAG? 
i saw unfoldMemoryOperand(for DAG)  in llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142084



More information about the llvm-commits mailing list