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

make AIR instruction for assembly not reference ZIR #10784

Closed
andrewrk opened this issue Feb 4, 2022 · 0 comments · Fixed by #10924
Closed

make AIR instruction for assembly not reference ZIR #10784

andrewrk opened this issue Feb 4, 2022 · 0 comments · Fixed by #10924
Assignees
@andrewrk
Labels
bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone
0.10.0

Comments

@andrewrk
Copy link
Member

andrewrk commented Feb 4, 2022

zig/src/Air.zig

Lines 665 to 669 in 1b6a1e6

pub const Asm = struct {
/// Index to the corresponding ZIR instruction.
/// `asm_source`, `outputs_len`, `inputs_len`, `clobbers_len`, `is_volatile`, and
/// clobbers are found via here.
zir_index: u32,

This is problematic because it causes

thread 345550 panic: access of inactive union field
/home/andy/dev/zig/src/Liveness.zig:434:83: 0x319d323 in Liveness.analyzeInst (zig)
            const extended = a.zir.instructions.items(.data)[extra.data.zir_index].extended;
                                                                                  ^

Problem is that the ZIR we pass to Liveness analysis (and to codegen later) is for a particular Decl that could have inlined code, causing an assembly ZIR instruction from a different file to be in the AIR for the Decl. This means we are looking up a ZIR instruction index in the wrong ZIR object.

This is the only AIR instruction that references a ZIR instruction.

The solution is pretty straightforward: instead of trying to cut corners, change the AIR encoding to duplicate all the info from the ZIR that it needs, and uphold the property that ZIR is not needed in order to decode AIR.
@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Feb 4, 2022
@andrewrk andrewrk added this to the 0.10.0 milestone Feb 4, 2022
andrewrk added a commit that referenced this issue Feb 5, 2022
@andrewrk
add Aro as a C frontend
376d443 
Problems to solve in this branch before merging:

 * Liveness and AIR printing want a ZIR instance. This issue is
   addressed by #10784.
 * linker: updateFunc / updateDecl want a Module instance which
   currently represents only Zig code. These functions should be changed
   to accept a Compilation, not a Module. The relevant state that it
   needed to touch on Module should be moved to Compilation instead.
 * `Decl` needs to be shared by both the C frontend and the Zig
   frontend. It currently has Zig-only fields and the functions to
   create Decl objects don't really fit the API that the C frontend
   needs.
 * A lot of the incremental compilation infrastructure doesn't quite
   match up, for example Decl objects being owned by Namespaces which is
   a Zig concept. Maybe there could be 1 Namespace globally across all C
   files and 1 more Namespace per C file for functions and globals
   declared `static`.
 * There is common code to be extracted from Sema that is shared with
   Aro frontend. All the helper functions having to do with the
   air_instructions and air_extra arrays.

There are many instances of calling `@panic("TODO")` that need to be
cleaned up.

There is a lot of compiler options that Zig is not communicating to Aro,
for example we do not pass the -D switches that we pass to Clang yet.
Need to audit `addCCArgs` and do the equivalent things for setting up
the Aro Compilation.

The Aro code is copied from https://github.com/Vexu/arocc, commit
7a0e54227e1ea2b2df8aa7bd2b7075f735109317. The only patches are to delete
Codegen and replace it with our own, and to delete main.zig, and add
Type and Value to lib.zig.

I believe the current plan is for main Aro development to happen
upstream on the arocc repository and periodically sync it downstream
with the Zig repository. It's up to @Vexu whether to keep that process
or change it in the future.
@andrewrk andrewrk mentioned this issue Feb 5, 2022
Closed
@andrewrk andrewrk self-assigned this Feb 18, 2022
andrewrk added a commit that referenced this issue Feb 19, 2022
@andrewrk
stage2: eliminate ZIR arg instruction references to ZIR
32edb9b 
Prior to this commit, the AIR arg instruction kept a reference to a ZIR
string index for the corresponding parameter name. This is used by DWARF
emitting code. However, this is a design flaw because we want AIR
objects to be independent from ZIR.

This commit saves the parameter names into memory managed by
`Module.Fn`. This is sub-optimal because we should be able to get the
parameter names from the ZIR for a function without having them
redundantly stored along with `Fn` memory. However the current way that
ZIR param instructions are encoded does not support this case. They
appear in the same ZIR body as the function instruction, just before it.
Instead, they should be embedded within the function instruction, which
will allow this TODO to be solved. That improvement is too big for this
commit, however.

After this there is one last dependency to untangle, which is for inline
assembly. The issue for that is #10784.
andrewrk added a commit that referenced this issue Feb 19, 2022
@andrewrk
stage2: make AIR not reference ZIR for inline assembly
4e1e5ab 
Instead it stores all the information it needs to into AIR.

closes #10784
@andrewrk andrewrk mentioned this issue Feb 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andrewrk andrewrk

Labels
bug Observed behavior contradicts documented or intended behavior enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Milestone
0.10.0
Development

Successfully merging a pull request may close this issue.

AIR independence day ziglang/zig
1 participant
@andrewrk