Skip to content

frontend: make fn calls byval; fix false positive isNonErr #16576

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

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Jul 27, 2023

This commit does two things which seem unrelated at first, but, together, solve a miscompilation, and potentially slightly speed up compiler perf, at the expense of making #2765 trickier to implement in the future.

Sema: avoid returning a false positive for whether an inferred error set is comptime-known to be empty.

AstGen: mark function calls as not being interested in a result location. This prevents the test case "ret_ptr doesn't cause own inferred error set to be resolved" from being regressed. If we want to accept and implement #2765 in the future, it will require solving this problem a different way, but the principle of YAGNI tells us to go ahead with this change.

Old ZIR looks like this:

%97 = ret_ptr()
%101 = store_node(%97, %100)
%102 = load(%97)
%103 = ret_is_non_err(%102)

New ZIR looks like this:

%97 = ret_type()
%101 = as_node(%97, %100)
%102 = ret_is_non_err(%101)

closes #15669

This commit does two things which seem unrelated at first, but,
together, solve a miscompilation, and potentially slightly speed up
compiler perf, at the expense of making #2765 trickier to implement in
the future.

Sema: avoid returning a false positive for whether an inferred error set
is comptime-known to be empty.

AstGen: mark function calls as not being interested in a result
location. This prevents the test case "ret_ptr doesn't cause own
inferred error set to be resolved" from being regressed. If we want to
accept and implement #2765 in the future, it will require solving this
problem a different way, but the principle of YAGNI tells us to go ahead
with this change.

Old ZIR looks like this:

  %97 = ret_ptr()
  %101 = store_node(%97, %100)
  %102 = load(%97)
  %103 = ret_is_non_err(%102)

New ZIR looks like this:

  %97 = ret_type()
  %101 = as_node(%97, %100)
  %102 = ret_is_non_err(%101)

closes #15669
@andrewrk andrewrk merged commit e661900 into master Jul 27, 2023
@andrewrk andrewrk deleted the enhance-error-sets branch July 27, 2023 17:12
@andrewrk
Copy link
Member Author

potentially slightly speed up compiler perf

Data point for building the compiler itself:

Benchmark 1 (3 runs): before/zig build-exe ...
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          60.5s  ±  223ms    60.3s  … 60.7s           0 ( 0%)        0%
  peak_rss           3.87GB ± 31.5MB    3.85GB … 3.91GB          0 ( 0%)        0%
  cpu_cycles          248G  ± 1.60G      246G  …  250G           0 ( 0%)        0%
  instructions        357G  ± 1.73G      356G  …  359G           0 ( 0%)        0%
  cache_references   14.4G  ± 15.8M     14.3G  … 14.4G           0 ( 0%)        0%
  cache_misses       1.15G  ± 45.6M     1.12G  … 1.20G           0 ( 0%)        0%
  branch_misses      1.67G  ± 12.2M     1.66G  … 1.68G           0 ( 0%)        0%
Benchmark 2 (3 runs): after/zig build-exe ...
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          61.9s  ± 1.83s     60.3s  … 63.9s           0 ( 0%)          +  2.4% ±  4.9%
  peak_rss           3.84GB ± 28.1MB    3.82GB … 3.87GB          0 ( 0%)          -  0.9% ±  1.7%
  cpu_cycles          248G  ±  818M      247G  …  249G           0 ( 0%)          +  0.0% ±  1.2%
  instructions        357G  ± 1.49G      356G  …  358G           0 ( 0%)          -  0.2% ±  1.0%
  cache_references   14.2G  ± 75.0M     14.1G  … 14.3G           0 ( 0%)          -  0.9% ±  0.9%
  cache_misses       1.17G  ± 67.1M     1.12G  … 1.25G           0 ( 0%)          +  1.6% ± 11.3%
  branch_misses      1.68G  ± 6.62M     1.68G  … 1.69G           0 ( 0%)          +  0.8% ±  1.3%

(insignificant)

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.

Error propagates with return x() but not with return try x() inside recursion
1 participant