Dart2JS pattern code less efficient than manual check.

[lrhn]

class C {
  final int? _x;
  C(this._x);

  @pragma("dart2js:never-inline")
  void manual() {
    var x = _x;
    if (x != null) print(x);
    else print("null");
  }

  @pragma("dart2js:never-inline")
  void pattern() {
    if (_x case var x?) print(x);
    else print("null");
  }

  @pragma("dart2js:never-inline")
  void promote() {
    if (_x != null) print(_x);
    else print("null");
  }
}

If I compile this with dart compile js -O4 --no-minify, the code for the pattern method is less efficient than the other two:

    manual$0() {
      var x = this._x;
      if (x != null)
        A.print(x);
      else
        A.print("null");
    },
    pattern$0() {
      var x, t1,
        _0_0 = this._x;
      if (_0_0 != null) {
        x = _0_0;
        t1 = true;
      } else {
        x = null;
        t1 = false;
      }
      if (t1)
        A.print(x);
      else
        A.print("null");
    },
    promote$0() {
      var t1 = this._x;
      if (t1 != null)
        A.print(t1);
      else
        A.print("null");
    }
  };

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.

[rakudrama]

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: #29475, #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.

[johnniwinther]

@rakudrama Do you have an alternative lowering in mind for this example that would help dart2js?

[rakudrama]

@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.

[rakudrama]

With the fix c634738, 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");
    },

[fishythefish]

Reopening due to revert: https://dart-review.googlesource.com/c/sdk/+/350006

[rakudrama]

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