Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark RTTI methods as const / pure, so compiler could optimize subsequent calls, if necessary. #5049

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Dec 3, 2024

No description provided.

@asl asl requested a review from vlstill December 3, 2024 20:26
@asl
Copy link
Contributor Author

asl commented Dec 3, 2024

Recently I noticed that compiler was unable to optimize multiple calls to ->to<T>() and ->is<T>() on the same node. It turns out that due to virtual calls it is in general takes a conservative approach.

With this PR we add pure / const attributes to corresponding typeid / isA and to implementation functions to correctly represent semantics.

Note that typeid and isA are const since they do not access any global state, their return values depend only on arguments. toImpl is pure as its return value depends on the global state, but only via this argument.

@asl asl added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Dec 3, 2024
…ent calls, if necessary.

Signed-off-by: Anton Korobeynikov <[email protected]>
@asl
Copy link
Contributor Author

asl commented Dec 11, 2024

@vlstill Will you please take a look when you will have a chance?

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me.

  • Should the annotations be also on the base methods in the RTTI::Base? I think that would make sense.
  • Can you add a comment why these attributes are helpful?

@asl
Copy link
Contributor Author

asl commented Dec 12, 2024

  • Should the annotations be also on the base methods in the RTTI::Base? I think that would make sense.

These methods are pure virtual there as they are part of interface. They technically might be non pure / const as this is essentially an implementation detail (though I doubt anyone would implement things differently, but still the interface allows this :) ) Note that RTTI::Base is never used as-is, it is expected that it is always a base class, so no declarations are visible there.

Other methods are non-virtual there and fully inlineable ad therefore there is no need for annotation there.

@asl asl requested a review from vlstill December 12, 2024 19:49
@asl
Copy link
Contributor Author

asl commented Dec 12, 2024

Comments were added

@vlstill
Copy link
Contributor

vlstill commented Dec 13, 2024

  • Should the annotations be also on the base methods in the RTTI::Base? I think that would make sense.

These methods are pure virtual there as they are part of interface. They technically might be non pure / const as this is essentially an implementation detail (though I doubt anyone would implement things differently, but still the interface allows this :) )

Yes, although the same is true for the implementation methods, as they can still be (in theory) overridden by non-pure/non-const versions, although that would probably violate some (at least implicit) requirements for RTTI methods. Therefore having the [[const]]/[[pure]] in the base "interface" might be a bit stronger declaration of the intend these functions should behave that way.

Note that RTTI::Base is never used as-is, it is expected that it is always a base class, so no declarations are visible there.

True, therefore from the compilator standpoint it probably doesn't matter. It is really hard to find any resources on handling [[const]]/[[pure]] on virtual functions though.

Other methods are non-virtual there and fully inlineable ad therefore there is no need for annotation there.

Yes, I had only the virtual methods in mind.


Overall, I am OK with having the attributes as is.

@asl asl added this pull request to the merge queue Dec 13, 2024
Merged via the queue into p4lang:main with commit 5dc7f87 Dec 13, 2024
19 checks passed
@asl asl deleted the pure-const-rtti branch December 13, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants