[PATCH] D157650: [RISCV] Add SchedRead for merge operand

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland created this revision.
michaelmaitland added reviewers: reames, craig.topper, wangpc.
Herald added subscribers: jobnoorman, luke, VincentWu, vkmr, frasercrmck, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, simoncook, johnrusso, rbar, asb, hiraditya, arichardson.
Herald added a project: All.
michaelmaitland requested review of this revision.
Herald added subscribers: llvm-commits, eopXD, MaskRay.
Herald added a project: LLVM.

Prior to this patch, SchedReads were not modeled for instructions that
did not have a MASK suffix. This patch adds a SchedRead for all
instructions that have a merge operand.

@reames is doing work to fold TA and TU instructions into a single
instruction. This means that a single instruction may or may not
actually read the merge operand (TU must read merge operand). It is
possible that a TA instruction needs to read the merge operand, for
instance if TA is implemented as TU. Therefore, it makes sense to
represent the read of the merge operand for both TA and TU instructions.

In the case that a subtarget wants to model TA read different from TU
read, the subtarget should use a SchedVariant, which has access to the
MachineInstr.

Without this patch, the current SchedReads are off by one for
instructions that have a merge operand.

I am concerned is that `forceMergeOpRead` is passed to `SchedXXX`, but
the `SchedXXX` is not defined next to the ins and outs. This leads us to
walk the class hierarchy to determine whether `forceMergeOpRead` needs to be
be true. I worry that this will make this change harder to review, and
it may not be clear that `forceMergeOpRead` needs to be updated if any
future changes add or remove merge operands. I thought about moving the SchedXXX
definitions to where the ins and outs occur (as a seperate patch), but the 
drawback of this is that we have to pass around lots of new arguments through
class heirarchies. One improvement on this is to use a single custom data
structure to pass through the heirarchies the eventually gets used by the
SchedXXX. If any reviewers care to share their own opinion on this concern, or
suggest a different solution to this concern, it would be greatly appreciated.
In any case, this concern is NFC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157650

Files:
  llvm/lib/Target/RISCV/RISCVInstrInfoV.td
  llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D157650.549153.patch
Type: text/x-patch
Size: 49367 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230810/8ca42a49/attachment-0001.bin>