[RISCV] Make CanLowerReturn protected for downstream maintenance

[Jim]

Summary

For the downstream RISCV maintenance, it would be easier to override and reuse CanLowerReturn for customizing.

[shiva0217]

Although most of the targets declare these functions as private, I think it might worth changing to public. So the various downstream RISCV users may able to inherit the class and customize the functions for their ISA and return the basic class functions to utilize the original infrastructure. Any thoughts?

[lenary]

Can you accomplish what you want to with protected rather than public? That would seem preferential I think.

[jrtc27]

I'm curious as to whether this is actually useful. At least for our CHERI fork, we are tightly integrated with the existing code, reusing bits here and there and hooking in where we need to override things. Are you really able to implement any meaningful extensions by overriding the functions without simply duplicating a lot of the logic (which, whilst it avoids merge conflicts, means that any changes and bug fixes likely don't get copied to your forked functions)?

LowerFormalArguments has a _lot_ of scaffolding I don't think you want to be duplicating. CanLowerReturn is a trivial wrapper around CC_RISCV that seems meaningless to override. LowerReturn and LowerCall similarly have a lot of support code that you probably don't want to touch. For us it's really CC_RISCV(_FastCC) that we touch, as well as a few of the (static) helpers called by the various functions. Do you have examples of cases where this change is a useful thing to have, beyond it conceptually being not incorrect to override them?

[Jim]

Only move CanLowerReturn to protected. In our case, we need to override this function for passing special type indirectly.
And the remaining type would be handled by the function of the basic class. It would be easier to integrate with the existing code.

[luismarques]

llvm/lib/Target/RISCV/RISCVISelLowering.h

164–169

It might be a good idea to add a comment explaining why it isn't private. Since it's only for the benefit of downstream users, it might not be obvious from looking around the mainline code, without looking at the reviews.

[Jim]

Add comment.

[lenary]

LGTM

[jrtc27]

I am still very confused why this is necessary. CanLowerReturn barely does anything other have a loop to call other functions that actually do useful work, it seems like the wrong place to hook into things. Your "explanation" is merely just "we want this because we want this". Could you please shed some light on how _exactly_ this is useful? As it stands I am none the wiser.

[asb]

Looks like this landed while I was composing the below:

It does look like most backends declare this as private rather than public. We've adopted that same convention, but I can't see a particularly coherent dividing line between which virtual methods (which are public in TargetLowering) are public/private in the derived class.

I think it would actually be better to go ahead and make this public (like it is in parent class) rather than establishing a precedent for protected (which is probably more confusing than the current situation as it's not clear on what basis overrides are public/private/protected). If you just make it public, I don't think we need a comment to explain why, but likely want to follow-up to better organise the overrides.

I see SystemZISelLowering.h is almost consistent in keeping overrides public (for some reason unwrapAddress is private). Unless there's a an argument in favour of making overrides private that I'm missing, I'd be open to a patch that moved all the overrides to public.

[Jim]

In D78545#2030863, @jrtc27 wrote:

I am still very confused why this is necessary. CanLowerReturn barely does anything other have a loop to call other functions that actually do useful work, it seems like the wrong place to hook into things. Your "explanation" is merely just "we want this because we want this". Could you please shed some light on how _exactly_ this is useful? As it stands I am none the wiser.

In CanLowerReturn, it calls CC_RISCV in a loop to check if the type can be returned directly. Actually, the change should be in CC_RISCV if we want a specific type returned indirectly.
But this change is maintained hard for downstream while we upgrade or merge the code from trunk. Let CanLowerReturn public/protected for overridden and reusing by the derived class without touching CC_RISCV.

[jrtc27]

In D78545#2030910, @Jim wrote:

In D78545#2030863, @jrtc27 wrote:

I am still very confused why this is necessary. CanLowerReturn barely does anything other have a loop to call other functions that actually do useful work, it seems like the wrong place to hook into things. Your "explanation" is merely just "we want this because we want this". Could you please shed some light on how _exactly_ this is useful? As it stands I am none the wiser.

In CanLowerReturn, it calls CC_RISCV in a loop to check if the type can be returned directly. Actually, the change should be in CC_RISCV if we want a specific type returned indirectly.
But this change is maintained hard for downstream while we upgrade or merge the code from trunk. Let CanLowerReturn public/protected for overridden and reusing by the derived class without touching CC_RISCV.

Yes, exactly, your code belongs in CC_RISCV, not CanLowerReturn. That's what we do for our fork, and that in and of itself rarely causes merge conflicts, because that code rarely needs to change upstream.

[lenary]

Sorry, this is my fault, I should have asked Jim to wait for more approvals.

@Jim please may you revert rGd775841d7d6ee3e8bbf3a420590be9bb19433eaa ?

[Jim]

@lenary Ok, I have reverted it.

[asb]

Please check D79928 which cleans up the visibility of all of these overridden methods.

llvm/lib/Target/RISCV/RISCVISelLowering.h

public:
 
   /// Returns the register with the specified architectural or ABI name. This
   /// method is necessary to lower the llvm.read_register.* and
   /// llvm.write_register.* intrinsics. Allocatable registers must be reserved
   /// with the clang -ffixed-xX flag for access to be allowed.
   Register getRegisterByName(const char *RegName, LLT VT,
                              const MachineFunction &MF) const override;
 
+protected:
+  // This method is protected for customizing and reusing by inherited class.
+  bool CanLowerReturn(CallingConv::ID CallConv, MachineFunction &MF,
+                      bool IsVarArg,
+                      const SmallVectorImpl<ISD::OutputArg> &Outs,
+                      LLVMContext &Context) const override;
+
 private:
   void analyzeInputArgs(MachineFunction &MF, CCState &CCInfo,
                         const SmallVectorImpl<ISD::InputArg> &Ins,
                         bool IsRet) const;
   void analyzeOutputArgs(MachineFunction &MF, CCState &CCInfo,
                          const SmallVectorImpl<ISD::OutputArg> &Outs,
                          bool IsRet, CallLoweringInfo *CLI) const;
   // Lower incoming arguments, copy physregs into vregs
   SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,
                                bool IsVarArg,
                                const SmallVectorImpl<ISD::InputArg> &Ins,
                                const SDLoc &DL, SelectionDAG &DAG,
                                SmallVectorImpl<SDValue> &InVals) const override;
-  bool CanLowerReturn(CallingConv::ID CallConv, MachineFunction &MF,
-                      bool IsVarArg,
-                      const SmallVectorImpl<ISD::OutputArg> &Outs,
-                      LLVMContext &Context) const override;
   SDValue LowerReturn(SDValue Chain, CallingConv::ID CallConv, bool IsVarArg,
                       const SmallVectorImpl<ISD::OutputArg> &Outs,
                       const SmallVectorImpl<SDValue> &OutVals, const SDLoc &DL,
                       SelectionDAG &DAG) const override;
   SDValue LowerCall(TargetLowering::CallLoweringInfo &CLI,
                     SmallVectorImpl<SDValue> &InVals) const override;
   bool shouldConvertConstantLoadToIntImm(const APInt &Imm,
                                          Type *Ty) const override {