RFC: make boundscheck a value

[vtjnash]

This PR turns the @boundscheck macro from being a special lexically scoped construct to being a normal value (Expr(:boundscheck) returns true, but can be replaced with false by inference). This lets us treat it as a normal Bool parameter during control flow, rather than requiring special handling:

 macro boundscheck(blk)
-    # hack: use this syntax since it avoids introducing line numbers
-    :($(Expr(:boundscheck,true));
-      $(esc(blk));
-      $(Expr(:boundscheck,:pop)))
+    return Expr(:if, Expr(:boundscheck), esc(blk))
 end

I'm opening this to try to get a sense for whether this seems like an improvement or not. I noticed that as we moved to a more-linear IR, we need to bracket many more statements Expr(:inbounds), so this also should makes the IR a bit more concise.

This design fixes #18261 also, since we no longer need to keep track of the lexical boundscheck scope of the callee while inlining (something that we don't do very well at now), just the inbounds scope of the caller (which was easy to add).

[ararslan]

hase -> has

[JeffBezanson]

This seems like a really good idea (as evinced by code deletion :) ). That boundscheck meta elim code was a nightmare.
Any data on IR size, sysimg size impact?

[tkelman]

should probably code highlight i and A here

[tkelman]

unaddressed review above

[tkelman]

should update devdocs. the meaning of different possible numbers of arguments here is worth documenting in comments and/or devdocs as it's not obvious

addressed, though there's a test/staged failure across multiple platforms, and how the variable numbers of arguments thread through src is still a little confusing

[tkelman]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[vtjnash]

hm, looks like some optimization passes weren't working right after some small changes to the AST. Let's try that again @nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[vtjnash]

Better, but still all jumbled up with the optimization change. Let's try again, now that that's on master: @nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[vtjnash]

OK, I don't see anything now that doesn't appear to be noise.

[oscardssmith]

You don't think the 7 ~8x slowdowns in sumcartesianrange are real?

[vtjnash]

No. Locally, I don't see a difference between them. However, I see it does have a run-to-run variation of up to 50x.

[tkelman]

That's likely a variance introduced by this branch or recently on master, I haven't seen that elsewhere. That seems like an undesirable loss in performance predictability, what's the underlying cause?

[vtjnash]

That variance number was computed on master

[vtjnash]

Let's see a comparison run anyways @nanosoldier runbenchmarks(ALL, vs=":master")

[alyst]

I'm not sure what exactly arrayset(true, ...) does, but isn't it safe to skip the bounds check here? The only case I can see is when _growend!() would fail to grow an empty array.

[vtjnash]

No, grow can also fail due to finalizers

[yuyichao]

The bound check doesn't really help catching that though....

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[oscardssmith]

That looks consistent to me.

[vtjnash]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[yuyichao]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan