Page MenuHomePhabricator

[RISCV] DAG nodes and pseudo instructions for CSR access
ClosedPublic

Authored by sepavloff on Fri, Mar 19, 3:14 AM.

Details

Summary

New custom DAG nodes were added to represent operations on CSR. These
nodes are lowered to corresponding pseudo instruction. Using the pseudo
instructions allows to specify different scheduling information for
operations on different system registers. It also make possible to
specify dependencies of instructions on specific system registers.

Diff Detail

Event Timeline

sepavloff created this revision.Fri, Mar 19, 3:14 AM
sepavloff requested review of this revision.Fri, Mar 19, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 19, 3:14 AM
Herald added a subscriber: MaskRay. · View Herald Transcript

This is an alternative implementation of the functionality implemented in D90853.

sepavloff updated this revision to Diff 331816.Fri, Mar 19, 3:29 AM

Use simm12 for numbers of system registers

CSR addresses are uimm12s not simm12s

llvm/lib/Target/RISCV/RISCVISelLowering.h
244

Hm, modify isn't a great name when WRITE_CSR exists

sepavloff updated this revision to Diff 331882.Fri, Mar 19, 8:02 AM

Updated patch

  • used uimm12 to represent system register number,
  • renamed 'modify' component of new names to 'swap',
  • removed prefix 'Pseudo' in new instructions.

CSR addresses are uimm12s not simm12s

Changed to new uimm12.

llvm/lib/Target/RISCV/RISCVISelLowering.h
244

Changed to swap.

So to provide alternate scheduling information, we need scheduler predicates to inspect the operands to find the system register?

And what pass would be responsible for adding implicit def/use of the rounding mode register?

sepavloff updated this revision to Diff 332286.Mon, Mar 22, 7:48 AM

Updated patch

  • Removed asm operands from Read_CSR and friends,
  • Added classes for defining operations on specific system registers.

So to provide alternate scheduling information, we need scheduler predicates to inspect the operands to find the system register?

Pseudos that operate on specific registers should get separate definitions, like:

def ReadFRM : ReadSysReg<SysRegFRM, [FRM]>;
def WriteFRM : WriteSysReg<SysRegFRM, [FRM]>;
def SwapFRM : SwapSysReg<SysRegFRM, [FRM]>;

And what pass would be responsible for adding implicit def/use of the rounding mode register?

I think the implicit use/def should be a property of FP instructions, but this is a topic for discussion (https://lists.llvm.org/pipermail/llvm-dev/2021-March/149177.html).

craig.topper added inline comments.Mon, Mar 22, 10:47 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
156

Does this need to inherit from Operand? Or need any of the Parser/Encoder/MCOperand etc? Could it just be an ImmLeaf down with simm32?

1134

Doesn't read need side effects to say after earlier writes to it?

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.

sepavloff updated this revision to Diff 332569.Tue, Mar 23, 2:07 AM

Updated patch

  • uimm12 is now only PatLeaf, not Operand.
  • set hasSideEffects for Read_CSR.

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.

There may be cases when using registers to express dependencies is not practical. For hypothetical example, if write to some system register results in cache invalidation or in memory translation change, it is easier to model such dependency as a side effect.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
156

Right now it may be ImmLeaf only. Changed the implementation properly.

1134

Indeed. Set hasSideEffects to true.

sepavloff edited the summary of this revision. (Show Details)Wed, Mar 31, 5:15 AM
asb added a comment.Wed, Mar 31, 5:49 AM

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.

In D98936#2661228, @asb 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.

In D98936#2661228, @asb 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 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.

In D98936#2661228, @asb 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 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?

In D98936#2661228, @asb 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 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.

In D98936#2661228, @asb 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 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.

sepavloff updated this revision to Diff 334455.Wed, Mar 31, 8:51 AM

Removed instructions Read_CSR, Write_CSR and Swap_CSR

evandro added inline comments.Thu, Apr 1, 8:54 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1138

Methinks that hasSideEffects = 0 is fine for reading CSRs, but not sure that there's some weird register out there that changes state after being read.

1146

This should definitely be hasSideEffects = 1.

1154

As should this be hasSideEffects = 1 too.

sepavloff added inline comments.Thu, Apr 1, 10:01 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1138

You are right. It is fairly possible and the previous version of this patch contained support for such general case (Read_CSR in https://reviews.llvm.org/D98936?vs=on&id=332569#toc). This pseudo is proposed for a particular case, which can be represented as a reading register. The concrete example is reading floating point control and state registers.

1146

Again, it is true in general case (see the removed pseudo Write_CSR in the previous version). This pseudo is for the special case, when write to system register has no effects other than changing content of the corresponding register. Floating point environment register behave exactly in this way.

1154

This is true for the removed Swap_CSR from the previous version. Maybe we need to put them back.

sepavloff updated this revision to Diff 334902.Thu, Apr 1, 11:24 PM

Rebased and added variants with immediate

Any feedback?

craig.topper added inline comments.Tue, Apr 6, 1:51 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
31

I think this should be SDT_RISCVReadCSR to following the convention established by the others.

1137

Should this be CSRRS to match the CSRR pseudo instruction definiton?

sepavloff updated this revision to Diff 335751.Wed, Apr 7, 1:33 AM

Addressed reviewer's notes and rebased

sepavloff marked 2 inline comments as done.Wed, Apr 7, 1:35 AM
sepavloff added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.td
31

Renamed.

1137

Changed the expansion.

This revision is now accepted and ready to land.Wed, Apr 7, 11:30 AM
This revision was automatically updated to reflect the committed changes.
sepavloff marked 2 inline comments as done.