Skip to content

Commit 2e17727

Browse files
committed
Clean up generated code: scalar-matrix broadcast, literal hoists, trailing whitespace
Three deferred micro-cleanups, held out of the op-table refactor's zero-snapshot-drift gate: - matrix(scalar, m, n) as an elementwise operand against a genuine rank-2 array now compiles to its scalar (native Fortran broadcast) instead of materializing an O(m*n) temporary; the claimed dims are still enforced -- compile error when statically wrong, runtime guard spelled from the dim expressions when symbolic. All other contexts keep the materialization paths, so no scalar with claimed array dims escapes. - hoist_unless_name() leaves literal constants alone (splicing a literal has no side effects), retiring runif()'s caller-side is.atomic() workaround and the temporaries floor()/ceiling()/%/% spent on constants. - The extern C signature joined newline-prefixed argument lines with ', ', leaving a trailing space on every line; it joins with a bare comma now. A test helper cat()-ing a space before newline is fixed the same way; the unused str_flatten_args() helper is deleted. Snapshot churn is mechanical: extern signatures lose their trailing space, two bind messages lose theirs. git diff --check is clean.
1 parent b6af748 commit 2e17727

42 files changed

Lines changed: 496 additions & 363 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

R/aaa-utils.R

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -274,16 +274,6 @@ str_flatten_commas <- function(...) {
274274
paste0(unlist(c(character(), ...), use.names = FALSE), collapse = ", ")
275275
}
276276

277-
str_flatten_args <- function(..., multiline = length(dots) >= 3) {
278-
dots <- unlist(c(character(), ...), use.names = FALSE)
279-
if (multiline) {
280-
dots <- paste0("\n ", dots, collapse = ",")
281-
paste(dots, "\n")
282-
} else {
283-
paste0(dots, collapse = ",")
284-
}
285-
}
286-
287277
interleave <- function(x, y) {
288278
stopifnot(is.atomic(x), is.atomic(y), length(y) == 1L, typeof(x) == typeof(y))
289279
drop_last(as.vector(rbind(x, y, deparse.level = 0L)))

R/c-wrapper.R

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -800,9 +800,13 @@ fsub_extern_decl <- function(fsub) {
800800
glue("{fsub_arg_var_c_type(var)} {var@name}__")
801801
}
802802
})
803-
if (length(fsub_c_sig) >= 3L) {
804-
fsub_c_sig <- paste0("\n ", fsub_c_sig)
803+
args_sig <- if (length(fsub_c_sig) >= 3L) {
804+
# one arg per line; join with a bare comma -- joining "\n "-prefixed
805+
# elements with ", " leaves a trailing space on every line
806+
paste0("\n ", fsub_c_sig, collapse = ",")
807+
} else {
808+
str_flatten_commas(fsub_c_sig)
805809
}
806810

807-
glue("extern void {fsub@name}({str_flatten_commas(fsub_c_sig)});")
811+
glue("extern void {fsub@name}({args_sig});")
808812
}

R/r2f-aab-core.R

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,20 @@ new_hoist <- function(scope) {
7777
}
7878

7979
# Hoist `x` into a temporary variable unless it already renders as a bare
80-
# variable name. Use this whenever the same operand is spliced into generated
81-
# code more than once: Fortran evaluates intrinsic actual arguments before the
82-
# call, so repeating an expression duplicates its side effects (e.g. RNG
83-
# state via runif()).
80+
# variable name or a literal constant. Use this whenever the same operand is
81+
# spliced into generated code more than once: Fortran evaluates intrinsic
82+
# actual arguments before the call, so repeating an expression duplicates
83+
# its side effects (e.g. RNG state via runif()) -- which names and literals
84+
# don't have.
8485
hoist_unless_name <- function(x, hoist) {
8586
stopifnot(inherits(x, Fortran), inherits(x@value, Variable))
8687
code <- trimws(as.character(x))
8788
if (!is.null(x@value@name) && identical(code, x@value@name)) {
8889
return(x)
8990
}
91+
if (grepl("^-?[0-9]+(\\.[0-9]+)?(_c_(int|double))?$", code)) {
92+
return(x)
93+
}
9094
tmp <- hoist$declare_tmp(
9195
mode = x@value@mode,
9296
dims = x@value@dims,

R/r2f-operators.R

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,93 @@ cast_binop_operands <- function(spec, op, left, right) {
119119
)
120120
}
121121

122+
# Match `matrix(<scalar>, nrow, ncol)`: data a length-1 literal or a
123+
# declared scalar, no byrow/dimnames. Returns the matched arguments or
124+
# NULL. Used by compile_binop_operands() to lower the fill to a native
125+
# scalar broadcast instead of the O(nrow * ncol) temporary the matrix()
126+
# handler would otherwise materialize.
127+
matrix_scalar_fill_args <- function(e, scope) {
128+
if (!is.call(e) || !identical(e[[1L]], quote(matrix))) {
129+
return(NULL)
130+
}
131+
mc <- tryCatch(match.call(matrix, e), error = function(...) NULL)
132+
if (is.null(mc)) {
133+
return(NULL)
134+
}
135+
margs <- as.list(mc)[-1L]
136+
if (
137+
!setequal(names(margs), c("data", "nrow", "ncol")) ||
138+
any(map_lgl(margs, is_missing))
139+
) {
140+
return(NULL)
141+
}
142+
data <- margs$data
143+
data_is_scalar <- (is.atomic(data) && length(data) == 1L && !is.na(data)) ||
144+
(is.symbol(data) &&
145+
{
146+
var <- get0(as.character(data), scope)
147+
inherits(var, Variable) && passes_as_scalar(var)
148+
})
149+
if (!data_is_scalar) {
150+
return(NULL)
151+
}
152+
margs
153+
}
154+
155+
# Compile the two operands of an elementwise binary op. The one special
156+
# case: `matrix(scalar, m, n)` against a genuine rank-2 array broadcasts
157+
# natively -- compile just the scalar and enforce the claimed dims against
158+
# the other operand (compile error when statically wrong, runtime guard
159+
# when symbolic, spelled from the dim expressions since the fill has no
160+
# array to size()). Everything else compiles as written.
161+
compile_binop_operands <- function(args, scope, ..., hoist = NULL) {
162+
fills <- lapply(args, matrix_scalar_fill_args, scope = scope)
163+
fill_idx <- which(!map_lgl(fills, is.null))
164+
165+
if (length(fill_idx) == 1L && !is.null(hoist)) {
166+
j <- fill_idx
167+
other <- r2f(args[[3L - j]], scope, ..., hoist = hoist)
168+
fill_dims <- r2dims(list(fills[[j]]$nrow, fills[[j]]$ncol), scope)
169+
fill_dims_f <- map_chr(fill_dims, \(d) dims2f(list(d), scope))
170+
broadcastable <- inherits(other, Fortran) &&
171+
!is.null(other@value) &&
172+
other@value@rank == 2L &&
173+
!passes_as_scalar(other@value) &&
174+
!any(map_lgl(fill_dims, is_scalar_na)) &&
175+
all(nzchar(fill_dims_f)) &&
176+
!any(grepl(":", fill_dims_f, fixed = TRUE))
177+
if (broadcastable) {
178+
other_dims <- matrix_dims(other)
179+
for (axis in 1:2) {
180+
other_dim <- if (axis == 1L) other_dims$rows else other_dims$cols
181+
verdict <- check_elementwise_lengths(fill_dims[[axis]], other_dim)
182+
if (!verdict$ok) {
183+
stop(
184+
"elementwise matrix operations require matching dimensions",
185+
call. = FALSE
186+
)
187+
}
188+
if (verdict$unknown) {
189+
emit_quickr_error_if(
190+
glue("({fill_dims_f[[axis]]}) /= size({other}, {axis})"),
191+
"elementwise matrix operations require matching dimensions",
192+
hoist,
193+
scope
194+
)
195+
}
196+
}
197+
fill <- r2f(fills[[j]]$data, scope, ..., hoist = hoist)
198+
out <- list(fill, other)
199+
return(if (j == 1L) out else rev(out))
200+
}
201+
fallback <- r2f(args[[j]], scope, ..., hoist = hoist)
202+
out <- list(fallback, other)
203+
return(if (j == 1L) out else rev(out))
204+
}
205+
206+
lapply(args, r2f, scope, ..., hoist = hoist)
207+
}
208+
122209
# Render one table row over cast, shape-resolved operands. `var` is the
123210
# conformed result Variable (floor_divide branches on its mode and hoists
124211
# a quotient temporary with its dims).
@@ -164,7 +251,7 @@ compile_binop <- function(args, scope, ..., hoist = NULL) {
164251
return(Fortran(glue("({op}{x})"), Variable(x@value@mode, x@value@dims)))
165252
}
166253

167-
.[left, right] <- lapply(args, r2f, scope, ..., hoist = hoist)
254+
.[left, right] <- compile_binop_operands(args, scope, ..., hoist = hoist)
168255

169256
.[left, right] <- cast_binop_operands(spec, op, left, right)
170257

R/r2f-random.R

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@ r2f_handlers[["runif"]] <- function(args, scope, ..., hoist = NULL) {
1717
# R evaluates runif() bounds exactly once, but `min` is spliced twice below
1818
# and the implied-do re-evaluates the whole expression per element; hoist
1919
# non-trivial bounds (e.g. an impure runif(1)) so they are evaluated once.
20+
# (hoist_unless_name() leaves names and literals alone.)
2021
bound <- function(r_arg) {
21-
b <- r2f(r_arg, scope, ..., hoist = hoist)
22-
if (is.atomic(r_arg)) b else hoist_unless_name(b, hoist)
22+
hoist_unless_name(r2f(r_arg, scope, ..., hoist = hoist), hoist)
2323
}
2424

2525
if (default_min && default_max) {

tests/testthat/_snaps/bind.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
Code
44
capture_bind_error(r2f(bad_cbind))
55
Output
6-
cbind() only supports rank 0-2 inputs
6+
cbind() only supports rank 0-2 inputs
77
Code
88
capture_bind_error(r2f(bad_rbind))
99
Output
10-
rbind() only supports rank 0-2 inputs
10+
rbind() only supports rank 0-2 inputs
1111

tests/testthat/_snaps/blas-guards.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,10 @@
6161
6262
6363
extern void fn(
64-
const double* const m__,
65-
const double* const x__,
66-
double* const out___,
67-
const R_xlen_t x__len_,
64+
const double* const m__,
65+
const double* const x__,
66+
double* const out___,
67+
const R_xlen_t x__len_,
6868
char* quickr_err_msg);
6969
7070
SEXP fn_(SEXP _args) {

tests/testthat/_snaps/block-scopes.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,9 @@
127127
128128
129129
extern void fn(
130-
const double* const x__,
131-
double* const out__,
132-
const R_len_t x__dim_1_,
130+
const double* const x__,
131+
double* const out__,
132+
const R_len_t x__dim_1_,
133133
const R_len_t x__dim_2_);
134134
135135
SEXP fn_(SEXP _args) {

tests/testthat/_snaps/c-bridge-hoist.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,10 @@
5252
5353
5454
extern void fn(
55-
const int* const n__,
56-
const int* const m__,
57-
const double* const a__,
58-
const double* const b__,
55+
const int* const n__,
56+
const int* const m__,
57+
const double* const a__,
58+
const double* const b__,
5959
double* const out__);
6060
6161
SEXP fn_(SEXP _args) {

tests/testthat/_snaps/closure-hoist-snapshots.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@
6868
6969
7070
extern void fn(
71-
const double* const x__,
72-
double* const out__,
71+
const double* const x__,
72+
double* const out__,
7373
const R_xlen_t x__len_);
7474
7575
SEXP fn_(SEXP _args) {
@@ -166,8 +166,8 @@
166166
167167
168168
extern void fn(
169-
const int* const nx__,
170-
const int* const ny__,
169+
const int* const nx__,
170+
const int* const ny__,
171171
double* const temp__);
172172
173173
SEXP fn_(SEXP _args) {

0 commit comments

Comments
 (0)