Skip to content

[13/15] Generated code carries avoidable temporaries and trailing whitespace on every wrapper line #134

Description

@mns-nordicals

What happens

Three cosmetic-but-real warts in emitted code. None of them produces a wrong
answer; all of them make the generated Fortran and C harder to read and
slightly more expensive than it needs to be.

1. Every argument line of a generated extern declaration ends in a
space.
fsub_extern_decl() in R/c-wrapper.R prefixes each argument with
"\n " and then joins the list with ", ", so the separator's space lands
at end-of-line:

extern void fn(
  const double* const a__, 
  const double* const b__, 
  double* const out___, 
  char* quickr_err_msg);

This is present on main today: 280 lines across the committed snapshots end
in a comma followed by a space.

2. matrix(scalar, m, n) used as an elementwise operand materializes the
whole matrix.
x + matrix(1, n, n) allocates and fills an n×n temporary
just to add a constant to every element:

allocate(tmp(n, n))
tmp = 1.0_c_double
out_ = (x + tmp)

Fortran broadcasts a scalar against an array natively, so the temporary is
pure overhead — O(m·n) memory and writes for a value that is one constant.

3. Literal constants are hoisted into named temporaries. This one is
self-inflicted by this series: PR 1 added a shared hoist_unless_name()
helper in R/r2f-aab-core.R to stop floor()/ceiling()/runif() from
evaluating an argument more than once. It returns its argument unchanged only
when the argument renders as a bare variable name, so a literal like
2.5_c_double gets a temporary declared, assigned, and then used once:

tmp_ = 2.5_c_double
out_ = floor(tmp_)

Splicing a literal twice cannot duplicate a side effect, so the temporary buys
nothing. PR 1 papered over the worst case at the call site — runif() carries
its own is.atomic() check to skip the hoist — rather than teaching the hoist
about literals, which left the same waste in
floor()/ceiling()/%/%/matrix().

There is nothing to look at on main for this one: hoist_unless_name() does
not exist there. It is only observable on the branch beneath this PR.

Why it happens

The first is a plain formatting bug in the join, pre-existing on main.

The second and third are the deliberate leftovers of the earlier PRs. Both
change emitted text, and the operator-table refactor (PR 9) was gated on
zero snapshot churn precisely so that its neutrality could be proven by
byte-comparison — so any cleanup that necessarily churns snapshots had nowhere
to go until that gate was past. Item 3 is the narrower case: PR 1 introduced
the over-hoisting as the cost of fixing a real double-evaluation bug, and took
the local workaround instead of the general fix to keep its own diff small.
This PR is where both get paid off.

Expected behavior

  • extern signatures join with a bare comma, so no generated line ends in
    whitespace.
  • A matrix() call with scalar data meeting a genuine rank-2 operand
    compiles to the scalar itself, with the claimed dimensions still checked
    against the other operand — statically when they are known, with a runtime
    guard when they are symbolic. No scalar carrying claimed array dims may
    escape into any other context.
  • hoist_unless_name() leaves literal constants alone, which also lets
    runif() drop its caller-side workaround.

Because all three change emitted text, they belong in one deliberate
snapshot-refreshing PR whose diff can be read as "mechanical", rather than
being mixed into a behavior change where real churn and cosmetic churn would
be indistinguishable.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions