[RISCV] Add SchedRead for merge operand
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. Differential Revision: https://reviews.llvm.org/D157650
Loading
Please sign in to comment