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

gccrs const folding: port over cp_walk_subtrees() #1286

Merged
merged 1 commit into from Jun 14, 2022

Conversation

abbasfaisal
Copy link
Collaborator

@abbasfaisal abbasfaisal commented May 29, 2022

Card: Link

Most of TREE_CODEs in the original function belonged to cp/cp-tree.def which have been removed so this ported function is a lot smaller.

@CohenArthur
Copy link
Member

Card: Link

This PR is WIP and opened as RFC.

* This copies over all the macros in cp-tree.h to rust-tree.h as this makes it easy to port further stuff from cp. This compiles even if the functions and structs inside the unused macros haven't been ported yet. The problem this would leave us with is it will make it difficult to change these to be look relevant to rust rather than c++. I think it would be easy to do that if we ported them one by one. Thoughts?

Agreed. I think this change is really big, and I don't mind it being merged, but I fear it might be hard to work with. I think an approach that would be better for you and people reviewing PRs is adding changes incrementally, bringing over everything you need to make them work in the meantime. Of course, @philberty will have the last word as he has a better understanding of your project :)

* The template stuff relavant to cp_walk_subtrees() in cp has also been copied over. I guess we would need to remove it all?

I think you're right

* Other than above two, there is C++ specific stuff in struct definitions and functions that I guess will need to be reviewed line by line to weed out.

* [This ](https://github.com/Rust-GCC/gccrs/compare/master...abbasfaisal:master#diff-c53c9c947998d46ed9d083290a905468978cb0023c9cc4d52708c487b99b75b3R684) and [this ](https://github.com/Rust-GCC/gccrs/compare/master...abbasfaisal:master#diff-c53c9c947998d46ed9d083290a905468978cb0023c9cc4d52708c487b99b75b3R684) bit need a careful look.

* Most copied over things are still prefixed with cp. I will change this once above points are okayed.

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

This PR has given me alot to think about after a conversation with @dafaust I think the fact your ported over LAMBDA_EXPR is pretty interesting.

I hope you don''t mind letting me look at this for another day. My gut feeling is that we need to remove the stuff for traitsand cp binding levels and namespaces also templates oh and thunk functions.

Leave this PR as is for another day and I will finish the review I just want to think about a few things here since the LAMBDA_EXPR will be really useful for us but I want to know how tied up is that gimplification code with the tsubst stuff for templates.

gcc/rust/backend/rust-tree.cc Outdated Show resolved Hide resolved
gcc/rust/backend/rust-tree.cc Outdated Show resolved Hide resolved
@philberty philberty self-assigned this Jun 2, 2022
@philberty philberty added this to In progress in GSoC 2022 - Constexpr via automation Jun 2, 2022
@dafaust
Copy link
Collaborator

dafaust commented Jun 2, 2022

  • This copies over all the macros in cp-tree.h to rust-tree.h as this makes it easy to port further stuff from cp. This compiles even if the functions and structs inside the unused macros haven't been ported yet. The problem this would leave us with is it will make it difficult to change these to be look relevant to rust rather than c++. I think it would be easy to do that if we ported them one by one. Thoughts?

I agree, I think porting things one-by-one as we have use for them may be better. Skimming through the additions to rust-tree.h there are a lot of macros, struct members, bitfield elements etc. that I'm not sure will be relevant to rust, and I think going back to find and remove them later will be more trouble.

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

Hi @abbasfaisal apologies for the delay in response here, I really like how you were able to port a lot of this over. But I think as a rule of thumb we should ignore any code that requires you to port over the extra TREE_CODE's from the CPP front-end it would also include things like the bool is_auto function or CLASS_TEMPLATE etc.

We want to port over the walk tree code because this is part of the code to replace param_decls with the arguments from last I remember walking the constexpr code.

So for this I would expect you can just this branch and just start by removing the is_auto function for example and keep going until you didn't include any extra new TREE_CODEs

GSoC 2022 - Constexpr automation moved this from In progress to Review in progress Jun 7, 2022
Signed-off-by: Faisal Abbas <90.abbasfaisal@gmail.com>
@abbasfaisal
Copy link
Collaborator Author

@philberty As discussed offline:

  • I have removed the TREE_CODEs not in gcc/tree.h which has helped trim this function and remove most of the earlier ported stuff from rust-tree.h so this PR is now more concise.
  • It seems use of TREE_CODEs inside cp/cp-tree.def does not cause build error which caused the earlier confusion.

@CohenArthur CohenArthur requested review from dafaust and dkm June 13, 2022 08:40
@abbasfaisal abbasfaisal changed the title WIP gccrs const folding: port over cp_walk_subtrees() gccrs const folding: port over cp_walk_subtrees() Jun 13, 2022
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

I think this is a good start. It will make more sense once you start adding the proper entrypoint and see code evaluated by this function. I think it's good to merge this for now, and it will probably receive some changes soon. Good job :)

Copy link
Collaborator

@dafaust dafaust left a comment

Choose a reason for hiding this comment

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

Nice I think this looks good :)
Just one thing @abbasfaisal maybe you can update the original comment a bit to reflect the current changes, just because bors stuffs that message into the git log; I think some of the bullets are no longer relevant (for this specific merge at least) and might be a little confusing in the log.

@abbasfaisal
Copy link
Collaborator Author

Nice I think this looks good :) Just one thing @abbasfaisal maybe you can update the original comment a bit to reflect the current changes, just because bors stuffs that message into the git log; I think some of the bullets are no longer relevant (for this specific merge at least) and might be a little confusing in the log.

Done :)

@CohenArthur
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 14, 2022

Build succeeded:

@bors bors bot merged commit 25974ba into Rust-GCC:master Jun 14, 2022
GSoC 2022 - Constexpr automation moved this from Review in progress to Done Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants