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

compiler metrics #425

Merged
merged 72 commits into from
Aug 16, 2024
Merged

compiler metrics #425

merged 72 commits into from
Aug 16, 2024

Conversation

tckmn
Copy link
Contributor

@tckmn tckmn commented Jun 21, 2024

Some comments on the code as I'm looking the whole thing over now:

  • I have only just now realized the ipow example no longer compiles, because I factored out lots of hardcoded constants into functions like cost_op. This should be extremely easy to fix at the tactic automation level (but I'm making this pull request now in the interest of being able to see the code sooner).

  • In the previous code, the large constant bound on function calls was added twice in the semantics, once in the exec fbody judgment and once in the postcondition. I don't really understand why this is, but I kept them both (as cost_call_internal and cost_call_external). As far as I can tell, only one of these is needed, and putting it in either place works just as well.

pratapsingh1729 and others added 30 commits May 3, 2022 16:18
…etrics

Conflicts:
	compiler/src/compiler/FlatImp.v
	compiler/src/compiler/FlatToRiscvMetric.v
	compiler/src/compiler/FlattenExpr.v
	compiler/src/compiler/RegAlloc.v
	compiler/src/compiler/Spilling.v
side effects:
- breaks useimmediate
- fixes spilling
it seems i am trying to do too many things at once so i am committing
the shared things so that i do not have to worry about missing
dependencies due to commit order
The spilling phase adds some boilerplate inside functions and around
function calls, so we need to account for it by making things
artificially more expensive before the spilling phase.

In an effort to avoid monolithic commits, this is a partial commit, so
the code definitely doesn't compile in the current state since lots of
things changed as a side effect.

There are a few comments that say "cost_something constraint:", which
document the various constraints on the magic numbers chosen as upper
bounds for the costs (100, 95, 50). These should probably change to
better numbers at some point.
The pipeline now knows that metrics are preserved throughout.

Many further changes are needed in RegAlloc.v and UseImmediate.v, so
changes to theorems in those files are not included in this commit (and
it will therefore not compile by itself as a result).
@tckmn
Copy link
Contributor Author

tckmn commented Jul 31, 2024

@samuelgruetter I think this branch should be in a pretty good state to merge now! A few notes:

  • I addressed most of the comments here, with the exception of the mentions of removing cost_SCall_L and the cost_call_internal in the spilled call spec in the pipeline. These extra costs cannot be removed, because the spilling and RISC-V compilation stages add boilerplate code to the program itself. I've renamed them to cost_spill_spec and cost_compile_spec for clarity (I'm not sure if I realized this at the time, but using cost_call_internal for the spilling phase was extremely misleading, as I don't think it was actually semantically correct, it just happened to have numbers that worked).

  • There is now only one cost_call (rather than both cost_call_external and cost_call_internal).

  • With Andres's help, the Qed slowness in the ipow example is fixed 🎉


eapply Hexit.
(*Qed.*)
Admitted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not admit any lemmas. If there is partial work in a lemma that you would not like to delete but also don't want to finish right now, you can use Abort instead of Admitted, so that the lemma can't be used by accident.

Copy link
Contributor Author

@tckmn tckmn Aug 9, 2024

Choose a reason for hiding this comment

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

Whoops, I didn't realize the WIP loop lemmas had made it into the PR; I deleted everything in that file that isn't finished (including the commented out stuff), possibly to be added back later now added back with correct proofs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(pinging @samuelgruetter since i believe there are no remaining outstanding comments)

@samuelgruetter samuelgruetter merged commit 7b611b6 into mit-plv:master Aug 16, 2024
4 checks passed
@samuelgruetter
Copy link
Contributor

Merged, thanks @tckmn 🎉

For the record, there are still a few points that would be nice to address in a later PR:

  • cost_interact, cost_call, cost_load etc should not take an mc argument. Instead, its callers should add mc to them
  • "TODO XXX address inconsistency in where metrics are added"
  • locals_based_call_spec vs locals_based_call_spec_spilled: are both needed?
  • cost_spill_spec and cost_compile_spec should not appear in the toplevel theorems, but should be part of the costs that appear in the semantics -- or what exactly do you mean by "These extra costs cannot be removed, because the spilling and RISC-V compilation stages add boilerplate code to the program itself"?

But to avoid that this PR goes stale and to make sure others can work on top of this, I now merged this PR.

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.

5 participants