This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use a ComplexPattern to merge isel patterns for vector load/store with GPR and FrameIndex addresses.
ClosedPublic

Authored by craig.topper on Feb 1 2021, 7:43 PM.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 1 2021, 7:43 PM
craig.topper requested review of this revision.Feb 1 2021, 7:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 7:43 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

I understand it is not a win to apply a similar scheme to the base RISCVInstrInfo.td and perhaps F's and D's because we do have an offset available there. Is my understanding correct?

For instance now we have in the base .td:

multiclass LdPat<PatFrag LoadOp, RVInst Inst> {
  def : Pat<(LoadOp GPR:$rs1), (Inst GPR:$rs1, 0)>;
  def : Pat<(LoadOp AddrFI:$rs1), (Inst AddrFI:$rs1, 0)>;
  def : Pat<(LoadOp (add GPR:$rs1, simm12:$imm12)),
            (Inst GPR:$rs1, simm12:$imm12)>;
  def : Pat<(LoadOp (add AddrFI:$rs1, simm12:$imm12)),
            (Inst AddrFI:$rs1, simm12:$imm12)>;
  def : Pat<(LoadOp (IsOrAdd AddrFI:$rs1, simm12:$imm12)),
            (Inst AddrFI:$rs1, simm12:$imm12)>;
}

I understand it is not a win to apply a similar scheme to the base RISCVInstrInfo.td and perhaps F's and D's because we do have an offset available there. Is my understanding correct?

For instance now we have in the base .td:

multiclass LdPat<PatFrag LoadOp, RVInst Inst> {
  def : Pat<(LoadOp GPR:$rs1), (Inst GPR:$rs1, 0)>;
  def : Pat<(LoadOp AddrFI:$rs1), (Inst AddrFI:$rs1, 0)>;
  def : Pat<(LoadOp (add GPR:$rs1, simm12:$imm12)),
            (Inst GPR:$rs1, simm12:$imm12)>;
  def : Pat<(LoadOp (add AddrFI:$rs1, simm12:$imm12)),
            (Inst AddrFI:$rs1, simm12:$imm12)>;
  def : Pat<(LoadOp (IsOrAdd AddrFI:$rs1, simm12:$imm12)),
            (Inst AddrFI:$rs1, simm12:$imm12)>;
}

I didn't realize how many times that is instantiated. We can probably at lest merge patterns 1 and 2 together, and 3 and 4 together. I ideally we could merge all of them with a ComplexPattern that emitted 2 results, but I think that requires changing the "ins" operands for the instructions and some assembly parser changes.

frasercrmck accepted this revision.Feb 2 2021, 1:28 AM

LGTM other than that stylistic change.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
859

I think the clang-tidy suggestion here is "correct" LLVM style: auto *

This revision is now accepted and ready to land.Feb 2 2021, 1:28 AM
This revision was landed with ongoing or failed builds.Feb 2 2021, 10:21 AM
This revision was automatically updated to reflect the committed changes.