This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support @llvm.readcyclecounter() Intrinsic
ClosedPublic

Authored by lenary on Jul 3 2019, 2:46 AM.

Details

Summary

On RISC-V, the cycle CSR holds a 64-bit count of the number of clock
cycles executed by the core, from an arbitrary point in the past. This
matches the intended semantics of @llvm.readcyclecounter(), which we
currently leave to the default lowering (to the constant 0).

With this patch, we will now correctly lower this intrinsic to the
intended semantics, using the user-space instruction rdcycle. On
64-bit targets, we can directly lower to this instruction.

On 32-bit targets, we need to do more, as rdcycle only returns the low
32-bits of the cycle CSR. In this case, we perform a custom lowering,
based on the PowerPC lowering, using rdcycleh to obtain the high
32-bits of the cycle CSR. This custom lowering inserts a new basic
block which detects overflow in the high 32-bits of the cycle CSR
during reading (because multiple instructions are required to read). The
emitted assembly matches the suggested assembly in the RISC-V
specification.

Diff Detail

Repository
rL LLVM

Event Timeline

lenary created this revision.Jul 3 2019, 2:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 2:46 AM
lebedev.ri added inline comments.
llvm/test/CodeGen/RISCV/readcyclecounter.ll
20 ↗(On Diff #207737)

If you change

-define i64 @test_builtin_readcyclecounter() {
+define i64 @test_builtin_readcyclecounter() nounwind {

you should get rid of this CFI noise

lenary updated this revision to Diff 207746.Jul 3 2019, 3:20 AM
  • Update test to add nounwind.
lenary marked 2 inline comments as done.Jul 3 2019, 3:21 AM
lenary added inline comments.
llvm/test/CodeGen/RISCV/readcyclecounter.ll
20 ↗(On Diff #207737)

Thanks for the tip!

lenary marked an inline comment as done.Jul 3 2019, 3:22 AM
lenary updated this revision to Diff 207748.Jul 3 2019, 3:26 AM
  • Run Clang-Format
asb requested changes to this revision.Jul 3 2019, 6:00 AM

Thanks Sam - left a number of comments in-line.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
160 ↗(On Diff #207748)

Couldn't hurt to add an assert here that that !Subtarget.is64Bit()

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
183 ↗(On Diff #207748)

Nit: should have full stop at end of comment

843 ↗(On Diff #207748)

Should add an assert that !Subtarget.is64Bit()

845 ↗(On Diff #207748)

RTB isn't really a meangful name. RCW?

1070 ↗(On Diff #207748)

Current style would be ReadMBB, though I'd be tempted to call this LoopMBB

1071 ↗(On Diff #207748)

Current style would be SinkMBB though I'd be temped to call this DoneMBB

1072 ↗(On Diff #207748)

Should be DL

1082 ↗(On Diff #207748)

When my select custom insertion was being reviewed, the consensus was that reassigning the BB pointer made the logic needlessly hard to follow for little benefit. I suggest just referring to LoopMBB directly instead.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1060 ↗(On Diff #207748)

Given we you have to list this as CSRRS, I'd phrase this as "we can directly access the 64-bit 'cycle' CSR."

1063 ↗(On Diff #207748)

"to the suggested loop reading the 'cycle' and 'cycleh' CSRs"?

1066 ↗(On Diff #207748)

Given opcodestr+argstr should't ever be emitted, I think it makes sense to just remove those strings.

This revision now requires changes to proceed.Jul 3 2019, 6:00 AM
simoncook added inline comments.Jul 3 2019, 6:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
843 ↗(On Diff #207748)

Is it worth having an assertion similar to ISD::SRL below that will trigger when this custom lower is called for rv64 subtargets?

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1062 ↗(On Diff #207748)

This should have a comment, like on line 611 which notes 0xc00 is a CSR address rather than a random constant.

(I also think it would be nice to have patterns select named InstAliases to use the rdcycle instruction alias directly, but it seems TableGen doesn't support doing such a thing)

lenary updated this revision to Diff 207822.Jul 3 2019, 9:53 AM

Address Review Feedback

  • Updated lots of comments
  • Added some asserts to trigger if we try to introduce or lower the custom pseudo-instruction on rv64.
  • Clarify some magic numbers

Potentially this should wait for D64139, so we can use the named
constants that PR introduces.

lenary marked 13 inline comments as done.
lenary added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1062 ↗(On Diff #207748)

I'm going to wait for D64139 to land, and then use the constants introduced there.

asb added inline comments.Jul 3 2019, 10:41 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1062 ↗(On Diff #207748)

Why not just rebase this patch on top of D64139?

lenary updated this revision to Diff 207997.Jul 4 2019, 2:42 AM

Incorporate changes based on D64139, in order to avoid magic numbers.

lenary marked 2 inline comments as done.Jul 4 2019, 2:52 AM
asb accepted this revision.Jul 5 2019, 1:47 AM

LGTM, thanks!

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
183 ↗(On Diff #207748)

New nit: reviewing this again, does this comment actually add value?

I'd actually consider a comment like:
`// TODO: On M-mode only targets, the cycle[h] CSR may not be present. Unfortunately this can't be determined just from the ISA naming string."

This revision is now accepted and ready to land.Jul 5 2019, 1:47 AM
lenary updated this revision to Diff 208131.Jul 5 2019, 3:04 AM
  • Update Comment
lenary marked an inline comment as done.Jul 5 2019, 3:05 AM
This revision was automatically updated to reflect the committed changes.