Open lrhn opened 10 months ago
dart2js has a structural problem that prevents it from generating nice code for examples like this. It will take significant significant effort to fix the problem, but I think we should do it.
The root cause is that dart2js was originally envisioned to be a more JIT-like compiler, so the CFG and translation from optimized CFG to JavaScript made a compromise that the CFG structure is immutable, and JavaScript generation was guided by the original Dart program structure. We have limped along with this for a long time. dart2js never generated nice code for complex conditionals (the condition becoming reified), and with more elaborate lowerings coming out of the CFE, this is no longer appropriate.
Fixing this in dart2js will be a significant effort since it will require rewrites of the code generator and pieces of the optimizer.
Being able to reshape the CFG is a necessary to doing better, but, as the similar issue you opened on the VM shows, it is not sufficient. The VM optimizer has excellent CFG reshaping capabilities.
If the CFE could generate better lowerings (there has been progress), that would help a lot with naive compilers like DDC.
This is the lowered Kernel IR:
class C extends core::Object {
final field core::int? _x;
constructor •(core::int? _x) → self::C
: self::C::_x = _x, super core::Object::•()
;
static method _#new#tearOff(core::int? _x) → self::C
return new self::C::•(_x);
@#C3
method manual() → void {
core::int? x = this.{self::C::_x}{core::int?};
if(!(x == null))
core::print(x{core::int});
else
core::print("null");
}
@#C3
method pattern() → void {
{
hoisted core::int x;
final synthesized core::int? #0#0 = this.{self::C::_x}{core::int?};
if(!(#0#0 == null) ?{core::bool} let final dynamic #t1 = x = #0#0{core::int} in true : false)
core::print(x);
else
core::print("null");
}
}
@#C3
method promote() → void {
if(!(this.{self::C::_x}{core::int?} == null))
core::print(this.{self::C::_x}{core::int?} as{Unchecked} core::int);
else
core::print("null");
}
}
See also: https://github.com/dart-lang/sdk/issues/29475, https://github.com/dart-lang/sdk/issues/7475
/cc @johnniwinther @alexmarkov @nshahan I don't think we should all try to solve this in our little silos. Perhaps we should meet somehow to figure out what we would need to allow the most straight-forward generation of better code.
@rakudrama Do you have an alternative lowering in mind for this example that would help dart2js?
@johnniwinther Sink binding as late as possible. If the binding occurs in one place, is not actually tested (as here) and not needed in more than one path, sink it into the 'then' part of the if-then-else.
Best case would be something like
method pattern() → void {
{
final synthesized core::int? #0#0 = this.{self::C::_x}{core::int?};
if(!(#0#0 == null) ?{core::bool}) {
core::int x = #0#0{core::int};
core::print(x);
} else
core::print("null");
}
}
but this might be OK too:
method pattern() → void {
{
hoisted core::int x;
final synthesized core::int? #0#0 = this.{self::C::_x}{core::int?};
if(!(#0#0 == null) ?{core::bool}) {
x = #0#0{core::int};
core::print(x);
} else
core::print("null");
}
}
A more general framing might be to push the irrefutable tail of a pattern into the 'then' part of the if-then-else.
With the fix c6347389d2c6953ab5447e3daa2a725406f16a4d, the generated code for pattern
is the same as manual
and promote
(other than names)
pattern$0() {
var _0_0 = this._x;
if (_0_0 != null)
A.print(_0_0);
else
A.print("null");
},
Reopening due to revert: https://dart-review.googlesource.com/c/sdk/+/350006
The issue causing the revert is addressed by https://dart-review.googlesource.com/c/sdk/+/352443
The branch condition in B4 is not controlling for the join at B12
If I compile this with
dart compile js -O4 --no-minify
, the code for thepattern
method is less efficient than the other two:It looks like some desugaring of patterns ends up reifying the match result as a boolean value, and introducing a temporary variable for the non-matching branch too, in a way that the Dart2JS compiler doesn't manage to optimize afterwards. (It seems like code that should be easily optimizable, so is optimization omitted for "lowered" Dart code?)
I'd prefer to use the
case
version, but only if it's actually as efficient.