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

irgen: Generate explicit instantiations #4681

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

ChrisDodd
Copy link
Contributor

  • explicit instantiations for all Vector and IndexedVector used in any .def file

@ChrisDodd ChrisDodd requested a review from vlstill May 27, 2024 08:43
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label May 27, 2024
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

This seems to be a better approach than #4680 because it automatically applies to all IR nodes with these characteristics.

I would add the comment/context from #4680 to the PR.

@vlstill can you confirm that this has a similar effect as your changes?

tools/ir-generator/irclass.cpp Outdated Show resolved Hide resolved
@vlstill
Copy link
Contributor

vlstill commented May 28, 2024

Sorry I got caught by other work. I have to check that this does not break anything is someone tried to instantiate (Indexed)Vector before including the ir-generated.h which now contains the extern declarations. I'm not sure what C++ says about such cases but it seems to be allowed.

EDIT: no problem there.

@vlstill
Copy link
Contributor

vlstill commented May 28, 2024

Hmm, I'm seeing only part of the improvement in this branch compared to my branch (after just one run, will confirm once the rest finishes). Could it be that for this to work fully the extern template declarations need to be before the use? Otherwise I think the template would still be (partially) instantiated with the class that uses it (e.g. member size needs to be known, constructor/destructor, things called from the ir-generated.h directly). And only after that the compiler would find the extern declaration.

Is it possible to move the declarations at the beginning, or is the ir-generator too much single-pass?

@vlstill
Copy link
Contributor

vlstill commented May 28, 2024

OK, so these are the results for release build (similar to #4674 (comment)):

# Original (#4681^ = this PR minus the last commit)
  Time (mean ± σ):     595.677 s ± 10.207 s    [User: 12206.865 s, System: 629.514 s]
  Range (min … max):   582.688 s … 609.877 s    5 runs

# Vector + IndexedVector manual (#4680, more precisely this RP minus the last commit + the commit from #4680)
  Time (mean ± σ):     541.040 s ± 11.445 s    [User: 11295.180 s, System: 602.781 s]
  Range (min … max):   523.936 s … 553.141 s    5 runs

# Vector + IndexedVector in ir-generator (#4681)
  Time (mean ± σ):     558.003 s ±  7.345 s    [User: 11216.258 s, System: 604.529 s]
  Range (min … max):   549.559 s … 568.076 s    5 runs

So this PR stands in-between the two previous benchmarks, although significantly closer to #4680. I agree this is the better approach overall (and downstream tools that have new IR nodes with Vector/IndexedVector will benefit more from it), but it would be good to move the declarations at the beginning to see if that helps (or maybe put them in a separate header and include that one directly to vector.h/indexed_vector.h.

@ChrisDodd
Copy link
Contributor Author

Is it possible to move the declarations at the beginning, or is the ir-generator too much single-pass?

I'll try moving them earlier, but I think the instantiations need to be after the declarations of the classes that are used as parameters to the template?

@ChrisDodd ChrisDodd force-pushed the cdodd-irgen branch 2 times, most recently from 660adb3 to 9774e21 Compare May 29, 2024 02:07
- explicit instantiations for all Vector and IndexedVector used in any
  .def file
@ChrisDodd
Copy link
Contributor Author

Is it possible to move the declarations at the beginning, or is the ir-generator too much single-pass?

Ok, I've moved the extern template declarations to the top of the ir-generated.h file -- this puts them before all the class definitions (and uses of the templates) -- see if that affects the build speed.

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.

Now the results are pretty much the same:

# this PR
  Time (mean ± σ):     566.911 s ±  3.272 s    [User: 11686.466 s, System: 588.923 s]
  Range (min … max):   562.361 s … 570.533 s    5 runs

# my approach
  Time (mean ± σ):     564.324 s ±  3.548 s    [User: 12161.493 s, System: 618.834 s]
  Range (min … max):   561.535 s … 570.435 s    5 runs

(I had to re-measure both as the PR base moved).

So the same. Thank you for this.

@ChrisDodd ChrisDodd added this pull request to the merge queue May 30, 2024
Merged via the queue into p4lang:main with commit 675d5bc May 30, 2024
17 checks passed
@ChrisDodd ChrisDodd deleted the cdodd-irgen branch May 30, 2024 04:50
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.

3 participants