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

Introduce -faggressive-inlining #469

Closed
wants to merge 1 commit into from

Conversation

mtzguido
Copy link
Member

Hi Jonathan, thanks for the tip yesterday about the inlining phase! It does make our code look much nicer.

In fact, I was thinking I would like this to be always done regardless of name and without requiring any attributes, so thought about adding this option, do you think it would be useful for others?

I could also add an attribute-based mechanism like you suggested if that's useful. If so, and if this option isn't desirable, maybe I can just have Pulse always add the attribute.

@msprotz
Copy link
Contributor

msprotz commented Aug 22, 2024

Personally, I'd rather have an attribute, since currently we're doing a bunch of hacks in HACL-rs to name some variables uu_ to avoid intermediary let-bindings that cause move-outs in Rust (see https://github.com/hacl-star/hacl-star/blob/afromher_rs/code/streaming/Hacl.Streaming.Functor.fst#L270 for instance). It would be cleaner if we could mark these let-bindings as [@inline] with a descriptive name (instead of uu_).

But I also appreciate that the "clean" solution is probably a little bit of work to propagate, so I'm fine with the flag too. Feel free to merge is that makes a significant difference for you (with the understanding that none of my projects will use this flag, so you'll have to keep an eye on it to make sure it doesn't regress).

@mtzguido
Copy link
Member Author

Sounds good, let me try that and get back.

@mtzguido mtzguido force-pushed the aggressive_inlining branch from 11fd091 to 606fa9c Compare August 30, 2024 16:53
@mtzguido
Copy link
Member Author

This was superseded by #470. I don't really need the flag anymore, hence closing, but I rebased it just in case it becomes useful at a point. We could also consider an F* flag to always add the attribute.

@mtzguido mtzguido closed this Aug 30, 2024
@mtzguido mtzguido deleted the aggressive_inlining branch September 7, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants