Some shapes are not rendering in release/profile Flutter >= v1.5.4-hotfix.2

[luigi-rosso]

Steps to Reproduce

Run one of the examples provided in:
2d-inc/Flare-Flutter#87
2d-inc/Flare-Flutter#92

1. Everything looks right in debug mode.
2. Run it again in release/profile.
3. Notice certain shapes are missing.

This is happening on stable v1.5.4-hotfix.2, dev, and master. The issue disappears if you revert to Flutter 1.2.1.

[jason-simmons]

Traced this to dart-lang/sdk@a10b0d8

@a-siva @aartbik

[aartbik]

I tried v1.6.1-pre.80, and I see an animation for debug, profile, and release.
Likewise I tried dev, v1.6.0 and see the animation. This is on an ARM64 device.

[jason-simmons]

It looks like this only affects ARM32 targets.

[aartbik]

@jason-simmons confirmed ARM64 works fine. Forcing it to ARM32 (on an ARM64 device) reproduces for me! Let the debugging begin!

[localpcguy]

I was seeing this on 64bit ARM devices (both on a Pixel XL and a Pixel 3).

[aartbik]

I traced this to a faulty CSE during AOT, not caused but exposed by given CL.

[aartbik]

Actually, the culprit CSE (1 out of 320K) does not look so offending after all. So either it exposes a bug downstream or some floating-point precision issue.

B204:
    ....
    v1773 <- DoubleToFloat:16(v841) T{_Double}
    ....
    v1862 <- FloatToDouble(v1773 T{_Double}) T{_Double}
    ....
    v893 <- FloatToDouble(v1773 T{_Double}) T{_Double}
    .....
    v222 <- BinaryDoubleOp(/, v893, v873 T{_Double}) T{_Double}

After CSE (v893 is replaced by v1862):

B204:
    ....
    v1773 <- DoubleToFloat:16(v841) T{_Double}
    ....
    v1862 <- FloatToDouble(v1773 T{_Double}) T{_Double}
    ....
    v222 <- BinaryDoubleOp(/, v1862 T{_Double}, v873 T{_Double}) T{_Double}

[aartbik]

Debugging had to go all the way down to arm assembly.
Good version sees

vmovq q1, q7
vmovq q7, q0
vcvtds d4, s28

bad version sees the somewhat baffling

vmovq q7, q7
vmovq q7, q0
vmovq q0, q7
vcvtds d2, s28

which is actually introduced by the neon version for ParallelMoveResolver::EmitSwap() in flow_graph_compiler_arm.cc which seems to over extend the usage of QTMP (letting the compiler fall back to the non-neon code does not fix the bug, it is simply a register availability problem):

     const QRegister dst = destination.fpu_reg();
     const QRegister src = source.fpu_reg();
     __ vmovq(QTMP, src);
     __ vmovq(src, dst);
     __ vmovq(dst, QTMP);

Asserting on dst and src both != QTMP indeed reveals the issue.

[aartbik]

@mraleph looping you in while I debug the register allocation today just in case you have some early insights

[mraleph]

@aartbik oh, this looks bad. I am surprised that we did not hit this before (I guess register pressure was not high enough to trigger Q<->Q swaps). We have code like this in il_arm.cc:

LocationSummary* FloatToDoubleInstr::MakeLocationSummary(Zone* zone,
                                                         bool opt) const {
  const intptr_t kNumInputs = 1;
  const intptr_t kNumTemps = 0;
  LocationSummary* result = new (zone)
      LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
  // Low (<= Q7) Q registers are needed for the conversion instructions.
  result->set_in(0, Location::FpuRegisterLocation(Q7));
  result->set_out(0, Location::RequiresFpuRegister());
  return result;
}

note that input 0 is using Q7. However Q7 is actually QTMP - so it must not be used as a fixed register like so, as it would interferes with internal usage. Seems like we missing some assertions to guard against that.

[aartbik]

I went over all the code and fixed all erroneous claims to Q7. This yields the now correct

vmovq q7, q6
vmovq q6, q0
vmovq q0, q7
vcvtds d2, s24

and fixes the bug! Everything animates well. I will push the fix and try to make a small reproducer to use as regression test. The improved CSE indeed increased register pressure.

[aartbik]

Fix has been submitted to Dart master. Note that it still needs an engine roll before it hits Flutter master.
@jason-simmons can you please verify the fix? Also, please advise if v1.5.4-hotfix.2 and dev need cherrypicks?

[jason-simmons]

Confirmed that the fix works on a local android_release build

[aartbik]

Fix has been rolled into the engine (ec4d48e241..50b0d85804), so I am closing this issue. Please advise if fix has to be cherrypicked elsewhere, or whether mainline fix suffices.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.