This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add SchedRead for merge operand
ClosedPublic

Authored by michaelmaitland on Aug 10 2023, 1:30 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 1:30 PM
michaelmaitland requested review of this revision.Aug 10 2023, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 1:30 PM

LGTM in general.
For your concern, I don't have any thought now. I will spend some time thinking about it.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 13 2023, 12:30 PM
This revision was automatically updated to reflect the committed changes.