For the downstream RISCV maintenance, it would be easier to override and reuse CanLowerReturn for customizing.
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?
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?
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.
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.
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.
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.
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.