[PATCH] D98936: [RISCV] DAG nodes and pseudo instructions for CSR access

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D98936#2661530 <https://reviews.llvm.org/D98936#2661530>, @sepavloff wrote:

> In D98936#2661518 <https://reviews.llvm.org/D98936#2661518>, @jrtc27 wrote:
>
>> In D98936#2661504 <https://reviews.llvm.org/D98936#2661504>, @sepavloff wrote:
>>
>>> In D98936#2661232 <https://reviews.llvm.org/D98936#2661232>, @jrtc27 wrote:
>>>
>>>> In D98936#2661228 <https://reviews.llvm.org/D98936#2661228>, @asb wrote:
>>>>
>>>>> In D98936#2642039 <https://reviews.llvm.org/D98936#2642039>, @jrtc27 wrote:
>>>>>
>>>>>> Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.
>>>>>
>>>>> Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.
>>>>
>>>> If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.
>>>
>>> The patch D99083 <https://reviews.llvm.org/D99083> demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.
>>
>> That looks like the kind of thing I'm imagining. So can we remove the Read/Write/Swap_CSR defs from this diff and just keep the Read/Write/SwapSysReg classes for use with such pseudos?
>
> No problem. I just thought that putting Read/Write/Swap_CSR defs (which are general purpose definitions) into a patch specific for FP support is not quite logical. But if this is OK, I'll move them.

I meant not having those defs at all, I don't see what they're useful for and are the pseudos I was taking issue with in my earlier comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98936