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

codegen: better support for openArray #756

Merged
merged 11 commits into from
Jun 12, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jun 11, 2023

Summary

Fix multiple issues related to the openArray type across all three
code generators. The intention is to make using the type stable
enough for the inlining logic in transf to use it as the type for
locals and object fields.

The unstable sink openArray feature stops working with the changes
here: its viewed elements are no longer destroyed when exiting the
routine accepting a sink openArray as a parameter.

Details

A big part of the changes is to treat var|lent openArray types the
same an unqualified openArray during the mid- and back-end phase of
compilation. In the future, the types should be lowered into openArray
prior to reaching the code generators.

The following list of changes is ordered by compilation phase, with the
earlier steps coming first.

Operator Lifting

openArray types are treated as primitive types, with a normal
assignment being used for copying and sinking, and destruction being a
no-op. This is necessary in order for copying the view to work.

This breaks sink openArray, which relied on a non-trivial destroy hook
that destroyed all element being generated for openArray.

MIR generation

Semantic analysis insert snkHiddenAddr and nkHiddenDeref nodes for
var openArrays. This is both unnecessary and confuses the code
generators, as no indirection is used and thus no taking the address /
dereferencing is needed.

While this ultimately needs to be fixed in sem, it is for now worked
around in mirgen: if the operand to nkHiddenAddr or nkHiddenDeref
is an openArray, the operator is ignored.

To make working with view types easier in the compiler's mid- and back-
end part, the classifyBackendView procedure is introduced, which for
the provided type computes whether it's a single-location view
(var|lent with non-openArray element), view of a sequence
(openArray), or no view.

Code generation

cgen

  • introduce the ctNimOpenArray enum field
  • map var|lent openArray to the same C type as an unqualified
    openArray
  • base genAssignment and constructLoc on the C type, instead of
    the NimSkull type, fixing assignment and initialization of
    var|lent openArrays
  • fix the C type-name caching logic to not cache var|lent openArray
    types

jsgen

  • implement default initialization and type info creation for
    openArray. This is required for openArrays to be part of compound
    types (arrays, tuples, objects)
  • include tyOpenArray in the list of "no copy" types
  • prevent a call to nimCopy during initialization when the destination
    is an openArray

vmgen

Refactor:

  • rename isVarLent to isLocView and use classifyBackendView
  • use classifyBackendView for isDirectView

Change:

  • isLocView only returns 'true' for single-location views
  • consider all direct views in isDirectView (openArray was missing)
  • consider all direct views (not only single-location ones) for
    assignments (genSymAsgn) and initialization (genVarStmt)

zerbina added 8 commits June 11, 2023 17:16
An openArray is a view; it doesn't need any special copy/sink/
destruction logic and can be treated like a primitive type.
Semantic analysis inserts `nkHiddenAddr` and `nkHiddenDeref` for
creation and access of `var|lent openArray` values. This is a problem
for the MIR and the code generators, as a `var|lent openArray` is (for
the largest part) the same as an `openArray` there.

`mirgen` now removes the unnecessary operations; the small formal
vs. actual type mismatch that results is acceptable for the time being.

This fixes assigning `seq`s, `array`s, etc. to `var openArray`
locations.
Outside of parameters, map a `var|lent openArray` to the same type as a
normal `openArray` (`ctStruct`). The special case in `getTypeDescAux`
becomes obsolete, and is removed.

This is the basic fix that most of the following C code-generator fixes
build upon.
* generate the correct type name (`openArrayType` instead of
  `openArrayType*`)
* don't cache the resulting C type

Together, this fixes non-parameter locations of `var|lent openArray`
type being declared with wrong C type.
Apart from simplifying the code and reducing the amount of nesting /
special cases, together with the introduction of `ctNimOpenArray`, this
fixes the wrong C code being generated for assignments to
`var|lent openArray` locations.
Similar to the previous change. This fixes `constructLoc` generating the
wrong C code for `var|lent openArray` location setup.
This fixes invalid code being generated when passing a `var openArray`
location to an openArray parameter.
Refactor:
* rename `isVarLent` to `isLocView` and use `classifyBackendView`
* use `classifyBackendView` for `isDirectView`

Fix:
* `isLocView` only returns true for single-location views
* consider all direct views in `isDirectView`
* consider all direct views (not only single-location ones) for
  assignments (`genSymAsgn`) and initialization (`genVarStmt`)

Creating local `openArray` views now works in a lot more cases.
@zerbina zerbina added bug Something isn't working refactor Implementation refactor compiler/backend Related to backend system of the compiler labels Jun 11, 2023
@zerbina zerbina added this to the MIR phase milestone Jun 11, 2023
@zerbina zerbina changed the title codegen: fix multiple openArray-related codegen: better support for openArray Jun 11, 2023
zerbina added 3 commits June 11, 2023 21:19
* consider `openArray` during type info generation (`genTypeInfo`)
* add default initialization support (`createVar`)
* include `tyOpenArray` in the list of "no copy" types
* on initialization, don't use a copy if the destination is a
  location of `openArray` type
@saem
Copy link
Collaborator

saem commented Jun 11, 2023

Memorializing thoughts at this point in time with respect to openArray (no change is required for this PR, just leaving a trail):

after coming back to openArray for so long and rereading the previous discussion. The only way sink openArray makes sense is if we treat openArray as a typeclass meaning any array/sequence/etc, and all parameter occurences make it generic and bind once as typeclasses do. While openArray[T] etc... are generic views, but that seems weird.

@@ -58,6 +58,11 @@ type
# most useful, shows: symbol + resolved symbols if it differs, e.g.:
# tuple[a: MyInt{int}, b: float]

BackendViewKind* = enum
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need this sort of categorization for views (and locations), starting it here is based on current needs seems sensible.

@saem
Copy link
Collaborator

saem commented Jun 12, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Jun 12, 2023
Merged via the queue into nim-works:devel with commit f71fa8d Jun 12, 2023
@zerbina zerbina deleted the backend-openarray-fixes branch June 12, 2023 14:21
@zerbina zerbina mentioned this pull request Dec 1, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 1, 2023
## Summary

Enable execution of `knownIssue` during CI test runs. This
automatically alerts about `knownIssue` tests starting to work, meaning
that one no longer has to check this manually.

Existing `knownIssue` tests that already work are updated or adjusted.

## Details

Execution of `knownIssue` tests is enabled by passing `--tryFailing` to
testament when testing in CI.

Multiple existing `knownIssue` tests already work, some also use the
`knownIssue` incorrectly. They're resolved in order for the test suite
to still pass.

Working tests:
* `tfragment_gc.nim` and `tfragment_alloc.nim` were both disabled due
  to out-of-memory failures caused by 4 GB allocations. The minimum
  amount of RAM provided by runners is currently 7 GB (according to the
 

[documentation](https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners)),
  meaning that the tests work
* `t02_argument_passing_mutate_bug.nim`: has always been working with
  the VM target
*  `t01_var_openarray.nim` : fixed by
#756
*  `tviews_in_object.nim` : fixed by
#769
*  `tdouble_evaluation_bug.nim`  fixed by
#828
* `t04_type_inference_fail_colon_expr.nim`: fixed by
  #1005 and replaced by
  `lang_exprs/tempty_typed_expressions.nim`

Tests that have been working prior to the most recent csources update
(~9 months ago):
* `t08_varargs_regular_subtype.nim`
* `tforloop_tuple_unpacking.nim`
* `tdata_serdes.nim`
* `tsequtils.nim`
* `tresults.nim` (for the JS target)

Tests that require specification changes:
* `t01_borrow_literal.nim`: the test tests for a compilation error, but
  was missing the `reject` specification
* `t07_addr_borrow_fail.nim`: the current language specification says
  that `addr x` is a path expression, but also that `ptr lent T` is not
  a view type. Given the unclear specification, the test is changed
  into testing for an error rather than success
* `t06_varargs_converter_clash.nim`: the test intends to ensure
  success, but the `errormsg` key means it does the opposite. The
  `errormsg` key is removed and turned into a comment instead
* `tmove_from_lvalue_conversion.nim`: doesn't rely on `--gc:orc` and
  already works with the JS and VM targets
* `thttpclient.nim`: the issue is with the test itself. It is disabled
  in addition to the `knownIssue` marker

Running `knownIssue` tests in the CI represents progress towards
#61.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler refactor Implementation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants