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

cgen: fix redundant initialization of temporaries #896

Merged
merged 3 commits into from
Sep 12, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Sep 12, 2023

Summary

Fix multiple places in the C code generator where temporaries inserted
by the C code generator (and not earlier) were redundantly initialized
twice.

While this is unlikely to have an impact on the performance of the
generated binaries, it nonetheless reduces the pressure on the C
compiler's optimizer, reduces the amount of work for the NimSkull
compiler, and also leads to the C artifacts becoming a bit smaller.

Details

The C code generator creates new temporary variables itself, usually for
the purpose of upholding evaluation order expectations. It allocates
them through cgen.getTemp, which allocates a name, emits the
declaration, and optionally (indicated by the needsInit parameter)
default-initializes the location.

Redundant initializations

  • for calls that use the return-value optimization and where the
    destination location is empty (fixupCall/genClosureCall; this
    happens, for example, for f(rvoCall())), the created temporary was
    initialized. This is redundant, however, as the called procedure is
    responsible for resetting the result location (if needed)
  • for potentially-raising calls that require a temporary to behave as
    expected (fixupCall), the temporary was initialized, but this is
    unnecessary, as its immediately assigned to after
  • for object constructions where the destination locations is empty
    (genObjConst; happens, for example, for f(Obj(...))), the type
    header fields were initialized twice. This is because getTemp calls
    constructLoc, which by default always initializes type headers

In addition, constructLoc (which was unconditionally used by
getTemp) always assigned the zero value to non-complex locations
(ints, floats, pointers, seqs, etc.), even if hasTemp is false,
causing many redundant assignments.

Improved handling

Apart from the aforementioned places where setting needsInit to 'true'
was unnecessary, there was only a single place where the needsInit
parameter was used (cgen.resetLoc). Therefore, the parameter plus
the constructLoc call are removed from getTemp; its usage sites are
now responsible for initializing the temporary. The isTemp parameter
for constructLoc is also not needed anymore and is thus removed.

All usage sites of getTemp are audited for whether they rely on the
initialization of scalars or type fields previously performed by
getTemp, but none does.

Temporaries created for holding the result of an RVO-using call were
default-initialized, but this is redundant: the called procedure already
takes care of resetting the result location as needed.
Temporaries created for holding the result value of a non-RVO call where
the destination location cannot be immediately assigned were
unnecessarily initialized; they're fully assigned to with the result of
the call.
* remove the `needsInit` parameter and `constructLoc` call from
  `getTemp` -- new temporaries always start uninitialized now
* remove the `isTemp` parameter from `constructLoc` -- `constructLoc`
  always zero'es complex locations now
* adjust `genDefault` to the `getTemp` changes

All usage sites of `getTemp` are audited for whether they depend on
the type-header initialization (`genObjectInit`), but none does.
@zerbina zerbina added refactor Implementation refactor compiler/backend Related to backend system of the compiler labels Sep 12, 2023
@zerbina zerbina added this to the C backend refactoring milestone Sep 12, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

One less boolean trap, nice!

@saem
Copy link
Collaborator

saem commented Sep 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

  • I noticed these while inspecting the generated C output
  • independent from other backend-related work

@chore-runner chore-runner bot added this pull request to the merge queue Sep 12, 2023
Merged via the queue into nim-works:devel with commit c5e1aa5 Sep 12, 2023
18 checks passed
@zerbina zerbina deleted the cgen-improve-temp-code-gen branch September 12, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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