Page MenuHomePhabricator

[RISCV] Make CanLowerReturn protected for downstream maintenance
ClosedPublic

Authored by Jim on Apr 21 2020, 1:41 AM.

Details

Summary

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

Diff Detail

Event Timeline

Jim created this revision.Apr 21 2020, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 1:41 AM

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?

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

jrtc27 added a comment.May 1 2020, 5:25 PM

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 updated this revision to Diff 262007.May 4 2020, 10:45 PM
Jim retitled this revision from [RISCV] Make Lower* functions public for downstream maintenance to [RISCV] Make CanLowerReturn protected for downstream maintenance.
Jim edited the summary of this revision. (Show Details)

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 added inline comments.May 6 2020, 7:04 AM
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 updated this revision to Diff 262552.May 6 2020, 10:32 PM

Add comment.

lenary accepted this revision.May 11 2020, 11:50 AM

LGTM

This revision is now accepted and ready to land.May 11 2020, 11:50 AM
This revision was automatically updated to reflect the committed changes.

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 added a comment.May 11 2020, 11:20 PM

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 added a comment.May 11 2020, 11:56 PM

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.

In D78545#2030910, @Jim 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.

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

@Jim please may you revert rGd775841d7d6ee3e8bbf3a420590be9bb19433eaa ?

Jim added a comment.May 12 2020, 4:18 AM

@lenary Ok, I have reverted it.

asb added a comment.EditedMay 14 2020, 2:16 AM

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