[PATCH] D85366: [RISCV] Disparage CSR instructions

Tue Aug 11 15:10:00 PDT 2020

jrtc27 added a comment.

In D85366#2211651 <https://reviews.llvm.org/D85366#2211651>, @evandro wrote:

> `hasSideEffects` may imply `isNotDuplicable`, especially when rematerializing, but the latter prevents duplication more extensively in the middle end (e.g., tail end duplication).

But why does that matter? If LLVM decides it's better to fork a code path then let it, whether it's a CSR instruction or not really does not matter, it's just I-cache usage. In fact that's another reason why I _wouldn't_ want this change to go through.

> Architecting reading the cycle counter to be like a GPR is not a given, since it's usually a side band register away from the register file.  Therefore, such instructions follow unusual paths in the architecture, when methinks that `hasNoSchedulingInfo` makes sense.

Did you read what I said? I didn't say it was a given. I said it was a possibility, and something that _should_ be done in a real high-end core, but no such implementation yet exists. The performance monitoring counters are _meant_ to be cheap to access per the spec ("Additional counters should be provided to help diagnose performance problems and these should be made accessible from user-level application code with low overhead.") and that's key to not perturbing the behaviour of the system by probing it. Yes, _some_ implementations (currently all that I know of) make them expensive to read. But that does not mean we should declare that _all_ implementations forever will, and having scheduling info is _key_ to being able to capture that. If your architecture makes them really expensive then go and tell LLVM that by writing a processor-specific schedule description. But don't try and enforce a one-size-fits-all approach for something where that's not true.

> Moreover, though the CSR instructions may be used as RO, they are defined as RMW instructions.  But this probably just backs up `hasSideEffects`.

Depends what you do with them. But even if they're RO, marking them hasSideEffects is I believe still important to ensure LLVM doesn't merge them as they're volatile reads.

> And not all code piped through the LLVM IR comes from `CodeGen`.  Thus, it's important to describe the instructions as faithfully as possible to the ISA, regardless of how they are used in the compiler.

Yes and no. There's currently no way to express IR that asks for anything other than RDCYCLE[H], so any frontend can only ever generate that; you'd have to hook into the MachineInstr bits and MC layer directly if you really wanted to emit anything else without the use of inline assembly (which is not subject to scheduling, so your flags wouldn't affect that) and that's really not encouraged, nor am I aware of anyone who does that. You are right though in the sense that we shouldn't assume RDCYCLE[H] will always be the only things emitted. But that potentially just means the performance counters should be separated out from the other CSRs if implementations exist (or are created) that behave differently between the two. Importantly, though, we should not sacrifice expressivity and flexibility by limiting all CSR instructions in this way. As I've said before and will keep saying, that is up to your machine scheduler, not the generic instruction info.

So I am still very against the entire concept of this patch. Before you reply next, please carefully read what I wrote last time and what I've written this time, because what you are saying leads me to believe you have not digested it; you're certainly not addressing the points I actually made.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D85366