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.

Sign up for GitHub

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

Jump to bottom

Sema: make @src().line comptime-known #17688

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Sema: make @src().line comptime-known #17688

merged 2 commits into from
Oct 24, 2023

Conversation

andrewrk
Copy link
Member

Reverts 89cef9f.

Closes #13315

image

We can figuring out how to deal with @src() in relationship with incremental compilation separately.

@andrewrk
Sema: make @src().line comptime-known
46a6d50 
Reverts 89cef9f.

Closes #13315
@andrewrk
tracy: protect source info via global constant
c01aa26 
When the code is written this way, you get a compile error if the
pointer given to Tracy does not have a static lifetime.

This would have caught the regression in #13315.
@mlugg
Copy link
Member

mlugg commented Oct 24, 2023

Note that this almost eliminates the need for the runtime_value value representation (representing a value which is really comptime-known but semantically runtime-known), which I think is desirable for language simplicity. The only place where this representation can still be introduced is in initialization expressions (looks for make_runtime in Sema.validateStructInit) as a result of the val.isPtrToThreadLocal check in resolveMaybeUndefValAllowVariablesMaybeRuntime. I don't really understand what's happening in that logic, but this should probably be looked into.

At a glance, I think we could inline the logic of resolveMaybeUndefValAllowVariablesMaybeRuntime into resolveMaybeUndefValAllowVariables, and change validateStructInit et all to use resolveMaybeUndefVal - I'm not actually sure what the AllowVariables is doing in these cases, it seems wrong. Then there are actually no correct usages of the AllowVariables variant at all (the only remaining ones are storeToInferredAllocComptime, which immediately exits if it's a variable, and zirClosureCapture, which can correctly just capture the type in this case). So that function also goes away!

It seems like there are quite a few dominos here - I'll explore locally and open a follow-up PR.

@mlugg mlugg mentioned this pull request Oct 24, 2023
Merged
@andrewrk andrewrk enabled auto-merge October 24, 2023 06:57
@andrewrk andrewrk merged commit b477279 into master Oct 24, 2023
10 checks passed
@andrewrk andrewrk deleted the comptime-src branch October 24, 2023 07:58
mlugg added a commit to mlugg/zig that referenced this pull request Aug 19, 2024
@mlugg
AstGen: incorporate extra information into source hashes
95a40bf 
* Indices of referenced captures
* Line and column of `@src()`

The second point aligns with a reversal of the "incremental compilation"
section of ziglang#2029 (comment).
This reversal was already done as ziglang#17688 (46a6d50), with the idea to
push incremental compilation down the line. My proposal is to keep it as
comptime-known, and simply re-analyze uses of `@src()` whenever their
line/column change.

I think this decision is reasonable for a few reasons:

* The Zig compiler is quite fast. Occasionally re-analyzing a few
  functions containing `@src()` calls is perfectly acceptable and won't
  noticably impact update times.
* The system described by Andrew in ziglang#2029 is currently vaporware.
* The system described by Andrew in ziglang#2029 is non-trivial to implement.
  In particular, it requires some way to have backends update a single
  global in certain cases, without re-doing semantic analysis. There is
  no other part of incremental compilation which requires this.
* Having `@src().line` be comptime-known is useful. For instance, ziglang#17688
  was justified by broken Tracy integration because the source line
  couldn't be comptime-known.
@mlugg mlugg mentioned this pull request Aug 19, 2024
Merged
mlugg added a commit to mlugg/zig that referenced this pull request Aug 20, 2024
@mlugg
AstGen: incorporate extra information into source hashes
1b0beba 
* Indices of referenced captures
* Line and column of `@src()`

The second point aligns with a reversal of the "incremental compilation"
section of ziglang#2029 (comment).
This reversal was already done as ziglang#17688 (46a6d50), with the idea to
push incremental compilation down the line. My proposal is to keep it as
comptime-known, and simply re-analyze uses of `@src()` whenever their
line/column change.

I think this decision is reasonable for a few reasons:

* The Zig compiler is quite fast. Occasionally re-analyzing a few
  functions containing `@src()` calls is perfectly acceptable and won't
  noticably impact update times.
* The system described by Andrew in ziglang#2029 is currently vaporware.
* The system described by Andrew in ziglang#2029 is non-trivial to implement.
  In particular, it requires some way to have backends update a single
  global in certain cases, without re-doing semantic analysis. There is
  no other part of incremental compilation which requires this.
* Having `@src().line` be comptime-known is useful. For instance, ziglang#17688
  was justified by broken Tracy integration because the source line
  couldn't be comptime-known.
mlugg added a commit to mlugg/zig that referenced this pull request Aug 21, 2024
@mlugg
AstGen: incorporate extra information into source hashes
43fdd06 
* Indices of referenced captures
* Line and column of `@src()`

The second point aligns with a reversal of the "incremental compilation"
section of ziglang#2029 (comment).
This reversal was already done as ziglang#17688 (46a6d50), with the idea to
push incremental compilation down the line. My proposal is to keep it as
comptime-known, and simply re-analyze uses of `@src()` whenever their
line/column change.

I think this decision is reasonable for a few reasons:

* The Zig compiler is quite fast. Occasionally re-analyzing a few
  functions containing `@src()` calls is perfectly acceptable and won't
  noticably impact update times.
* The system described by Andrew in ziglang#2029 is currently vaporware.
* The system described by Andrew in ziglang#2029 is non-trivial to implement.
  In particular, it requires some way to have backends update a single
  global in certain cases, without re-doing semantic analysis. There is
  no other part of incremental compilation which requires this.
* Having `@src().line` be comptime-known is useful. For instance, ziglang#17688
  was justified by broken Tracy integration because the source line
  couldn't be comptime-known.
richerfu pushed a commit to richerfu/zig that referenced this pull request Oct 28, 2024
@mlugg @richerfu
AstGen: incorporate extra information into source hashes
f6e0afb 
* Indices of referenced captures
* Line and column of `@src()`

The second point aligns with a reversal of the "incremental compilation"
section of ziglang#2029 (comment).
This reversal was already done as ziglang#17688 (46a6d50), with the idea to
push incremental compilation down the line. My proposal is to keep it as
comptime-known, and simply re-analyze uses of `@src()` whenever their
line/column change.

I think this decision is reasonable for a few reasons:

* The Zig compiler is quite fast. Occasionally re-analyzing a few
  functions containing `@src()` calls is perfectly acceptable and won't
  noticably impact update times.
* The system described by Andrew in ziglang#2029 is currently vaporware.
* The system described by Andrew in ziglang#2029 is non-trivial to implement.
  In particular, it requires some way to have backends update a single
  global in certain cases, without re-doing semantic analysis. There is
  no other part of incremental compilation which requires this.
* Having `@src().line` be comptime-known is useful. For instance, ziglang#17688
  was justified by broken Tracy integration because the source line
  couldn't be comptime-known.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

debug info missing from Tracy
2 participants
@andrewrk @mlugg