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

Address Spaces #9649

Merged
merged 41 commits into from
Sep 21, 2021
Merged

Address Spaces #9649

merged 41 commits into from
Sep 21, 2021

Conversation

Snektron
Copy link
Collaborator

@Snektron Snektron commented Aug 29, 2021

status:

  • Parsing addrspace on pointers, functions and variables.
  • Render address spaces in zig fmt
  • AstGen
    • Add addrspace property to pointers/functions/variables in ZIR.
    • Forbid addrspace on local variables
  • Sema
    • Update ZIR format to changes made in AstGen
    • decl_ref pointers
    • *?T => *T
    • *(!E|T) => *T
    • Pointer access chains (&x[y], &x.y, &x.?, &x[y].?[z..w].u, etc)
    • x[y..z]
    • Disallow coercing pointers between address spaces
    • Check whether address space is supported by target
    • Check whether address space is valid for variable
    • Check whether address space is valid for function
  • Implement address space attribute in the LLVM backend
  • Implement address space attribute in other backends
  • Allow pointers for address spaces to be different widths
  • Allow different values for null and allowzero depending on address space (see AMDGPU LLVM target)
  • Implement address space property in TypeInfo and make it work with @Type
  • Implement address space support for @export and @extern
  • Implement different default address space depending on context
  • Implement address spaces for targets:
    • x86 / x86_64
    • AVR
    • AMDGPU
    • NVPTX
    • SPIRV

Implements #653

@Snektron
Copy link
Collaborator Author

Snektron commented Aug 29, 2021

Currently, the default address space is .generic which is supposed to be supported (in some sense) by any target. A pointer pointing into the generic address space can be constructed either by *addrspace(.generic) T or by *T. While this does allow two different syntaxes for the same pointer, i believe that this helps with some generic programming and also mirrors how callconv(.Unspecified) is allowed.

Note that for some targets (like SPIR-V), there is also a specific address space called generic, which is not supported in the case of shaders. I don't think that's much of a problem though, and in that case the generic address space can simply be disallowed, and the user would be required to set the address space manually on all global variables.

Speaking of, some targets have different address spaces intended for different purposes. For example, on SPIR-V function-local variables are placed in a different address space than (instance specific) global variables. My plan was to infer the assigned address space from context, though that might yield some confusing code:

var a: i32 = 0; // inferred address space: .private
export fn b() void {
  var c: i32 = 0; // infered address space: .function
}

Currently i also forbid specifying an explicit address space on function-local variables, though i do not know whether SPIR-V actually permits locals in a different address space than .function. (Resolved: It does not, see SPIR-V spec section 2.4, under "Within a function definition").

@Snektron Snektron force-pushed the address-space branch 11 times, most recently from abf9478 to 05dfe25 Compare September 5, 2021 21:46
@Snektron Snektron force-pushed the address-space branch 3 times, most recently from 6ad38b4 to 83427ee Compare September 9, 2021 23:59
@Snektron
Copy link
Collaborator Author

At the moment there is no method to construct pointers to function in a different address space, as function pointers and declarations are kinda ambiguous in type.zig.

Copy link
Contributor

@matu3ba matu3ba left a comment

Choose a reason for hiding this comment

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

Mostly the doc comment stuff and minor nits regarding ordering of stuff.
Would be also great to have other address spaces than .generic tested (with stage2), so putting a TODO for "do this after selfhosted" might be good.

Do you have any plan/idea how and when this should be documented? Probably adding a stub in the docs would also be fine.
From what I am reading fs,gs and ss are used for OS-stuff and security-stuff, but I am unsure where this fits best (back-end specific stuff doesnt belong into the lang reference).

lib/std/zig/Ast.zig Outdated Show resolved Hide resolved
lib/std/zig/Ast.zig Show resolved Hide resolved
lib/std/zig/tokenizer.zig Show resolved Hide resolved
lib/std/zig/tokenizer.zig Show resolved Hide resolved
lib/std/zig/tokenizer.zig Show resolved Hide resolved
src/Module.zig Show resolved Hide resolved
The grammar for function prototypes, (global) variable declarations, and
pointer types now accepts an optional addrspace(A) modifier.
Adds AST generation for address spaces on pointers, function prototypes,
function declarations and variable declarations. In the latter two cases,
declaration properties were already stored more efficiently in a declaration
structure. To accomodate these for address spaces, the bit indicating presence
of a linksection attribute has been extended to include either linksection,
address space, or both.
This previously caused a test case to crash due to lingering llvm state.
Validity checks are also based on context; whether the entity being validated
is a mutable/constant value, a pointer (that is ascripted with an addrspace
attribute) or a function with an addrspace attribute. Error messages are
relatively simple for now.
This is a property which solely belongs to pointers to functions,
not to the functions themselves. This cannot be properly represented by
stage 2 at the moment, as type with zigTypeTag() == .Fn is overloaded for
for function pointers and function prototypes.
@Snektron
Copy link
Collaborator Author

Would be also great to have other address spaces than .generic tested (with stage2), so putting a TODO for "do this after selfhosted" might be good.

There also is a test in stage2/llvm.zig where the functionality of addrspace(.fs) is tested. Unfortunately these address spaces are hard to functionaly test, and are mainly useful on freestanding architectures.

Do you have any plan/idea how and when this should be documented? Probably adding a stub in the docs would also be fine.
From what I am reading fs,gs and ss are used for OS-stuff and security-stuff, but I am unsure where this fits best (back-end specific stuff doesnt belong into the lang reference).

addrspace is meant to also be used for other architectures, not x86 specifically. For example, AVR and GPU targets. I didn't think it was a good idea to also add these in this pr, but it does mean that the addrspace attribute itself is not a backend-specific feature.

As for documentation, I wasn't sure on that either, because this is of course a feature that is currently not supported by the stage 1 compiler. I can see a few options, though:

  • Add a note that this is a stage-2-only feature.
  • Backport to stage 1. I'm not sure whether this is worth the effort.
  • Wait until stage 1 is yeeted before merging this.

malcolmstill and others added 10 commits September 20, 2021 01:58
…128 encoding throughout its specification.

The WebAssembly spec requires signed LEB128 to be encoded up to a maximum number of bytes (max 5 bytes for i32, max 10 bytes for i64) and that "unused" bits are all 0 if the number is positive and all 1 if the number is negative. The Zig LEB128 implementation already enforces the max number of bytes and does check the unused bytes https://github.com/ziglang/zig/blob/master/lib/std/leb128.zig#L70-L79.

However, the WebAssembly test suite has a number of tests that were failing validation (expecting the wasm module to fail validation, but when running the tests, those examples were actually passing validation):

    https://github.com/malcolmstill/foxwren/blob/master/test/testsuite/binary-leb128.wast#L893-L902
    https://github.com/malcolmstill/foxwren/blob/master/test/testsuite/binary-leb128.wast#L934-L943

Notably the failures are both cases of negative numbers and the top 4 bits of the last byte are zero. And I believe this is the issue: we're only currently checking the "unused" / remaining_bits if we overflow, but in the case of 0x0_ no overflow happens and so the bits go unchecked.

In other words:

    \xff\xff\xff\xff\7f rightly successfully decodes (because it overflows and the remaining bits are 0b1111)
    \xff\xff\xff\xff\6f rightly errors with overflow (because it overflows and the remaining bits are 0b1110)
    \xff\xff\xff\xff\0f incorrectly decodes when it should error (because the top 4 bits are all 0, and so no overflow occurs and no check that the unused bits are 1 happens)

This PR adds a the remaining_bits check in an else branch of the @shlWithOverflow when we're looking at the last byte and the number being decoded is negative.

Note: this means a couple of the test cases in leb128.zig that are down as decoding shouldn't actually decode so I added the appropriate 1 bits.
* sync function arguments name with other same functions
This edit allows the reader to understand the syntax this section is talking about more quickly – they don’t have to read the whole code block and understand which part of it demonstrates the feature being described.

Affects https://ziglang.org/documentation/master/#Inferred-Error-Sets
 * introduce float_to_int and int_to_float AIR instructionts and
   implement for the LLVM backend and C backend.
 * Sema: implement `zirIntToFloat`.
 * Sema: implement `@atomicRmw` comptime evaluation
   - introduce `storePtrVal` for when one needs to store a Value to a
     pointer which is a Value, and assert it happens at comptime.
 * Value: introduce new functionality:
   - intToFloat
   - numberAddWrap
   - numberSubWrap
   - numberMax
   - numberMin
   - bitwiseAnd
   - bitwiseNand (not implemented yet)
   - bitwiseOr
   - bitwiseXor
 * Sema: hook up `zirBitwise` to the new Value bitwise implementations
 * Type: rename `isFloat` to `isRuntimeFloat` because it returns `false`
   for `comptime_float`.
also implement textual printing of the ZIR instruction `atomic_rmw`.
 * Sema: zirAtomicLoad handles 0-bit types correctly
 * LLVM backend: when lowering function types, elide parameters
   with 0-bit types.
 * Type: abiSize handles u0/i0 correctly
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Excellent work.

All the open questions and unchecked boxes can be separate issues.

There were two things to resolve here:
 * Snektron's branch edited Zir printing, but in master branch
   I moved the printing code from Zir.zig to print_zir.zig. So that
   just had to be moved over.
 * In master branch I fleshed out coerceInMemory a bit more, which
   caused one of Snektron's test cases to fail, so I had to add
   addrspace awareness to that. Once I did that the tests passed again.
@andrewrk andrewrk merged commit 1ad905c into ziglang:master Sep 21, 2021
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.

9 participants