Failures on [tests] Remove triple shift test skips from status files...Fix for accessing CompilationUnitElement.mixins without types.

[mkustermann]

There are new test failures on [tests] Remove triple shift test skips from status files...Fix for accessing CompilationUnitElement.mixins without types..

The tests

co19/Language/Functions/syntax_t03 Crash (expected Pass)

are failing on configurations

dartkp-strong-linux-debug-x64
dartkp-strong-linux-release-simarm64
dartkp-strong-linux-release-x64
dartkp-strong-win-release-x64
dartkp-weak-asserts-linux-debug-x64
dartkp-weak-asserts-linux-release-simarm64
dartkp-weak-asserts-linux-release-x64
dartkp-weak-asserts-win-release-x64

This hits an UNREACHABLE() in the VM and seems to be related to the new triple shift operator.

/cc @alexmarkov @a-siva

[alexmarkov]

Small repro:

@pragma('vm:never-inline')
void foo() {}

main() {
  int x = 1;
  int y = 10;

  foo();

  i_equality() => x != y;

  x = 3;
  x & y;
  x == y;
  i_equality();
}

This crash is not related to triple-shift operator.

[mkustermann]

Maybe enabling those tests (which were skipped due to triple shift) unveiled an existing issue then.

[mkustermann]

We should verify whether this happens also on the stable channel and if so possibly file a cherry-pick request.

Thanks for looking into it, @alexmarkov

[alexmarkov]

The following happens with != comparison:

1) ApplyClassIds pass converts it into CheckedSmiComparison and then it is inlined into main. 2) It uses results of LoadField instructions which yield nullable ints, so CheckedSmiComparison is not converted into EqualityCompare yet:

    v35 <- LoadField(v33 . x) T{int?}
    v37 <- LoadField(v33 . y) T{int?}
    v39 <- CheckedSmiComparison:12(!=, v35, v37) T{bool}

3) AllocationSinking_Sink pass forwards and removes loads:

    v9 <- Constant(#3) [3, 3] T{_Smi}
    ...
    v12 <- LoadField(v2 . y) T{int?}
    v26 <- CheckNull:22(v12, NoSuchMethodError) [-9223372036854775808, 9223372036854775807] T{int}
    v49 <- EqualityCompare(v9 == v26 T{int})
    v39 <- CheckedSmiComparison:12(!=, v9, v12 T{int?}) T{bool}

4) After that, next TypePropagation pass extends the effect of CheckNull by replacing v12 with v26:

    v12 <- LoadField(v2 . y) T{int?}
    v26 <- CheckNull:22(v12, NoSuchMethodError) [-9223372036854775808, 9223372036854775807] T{int}
    v39 <- CheckedSmiComparison:12(!=, v9, v26 T{int}) T{bool}

5) The next SelectRepresentations pass selects boxed representation for the inputs of CheckedSmiComparison. This is the last (the 2nd) SelectRepresentations pass.

6) The next Canonicalize converts CheckedSmiComparison into EqualityCompare because inputs finally become non-nullable ints after TypePropagation at step (4):

    v12 <- LoadField(v2 . y) T{int?}
    v26 <- CheckNull:22(v12, NoSuchMethodError) [-9223372036854775808, 9223372036854775807] T{int}
    v51 <- EqualityCompare(v9 != v26 T{int})

7) There are no more SelectRepresentations passes which could insert unboxing. Code generation crashes as it expects unboxed inputs of EqualityCompare instruction.

This looks like a pass ordering problem - there's no Canonicalize between TypePropagation and SelectRepresentations, and adding it fixes this problem. If we would like to keep TypePropagation immediately before SelectRepresentations, then maybe we should add extra TypePropagation and Canonicalize after the 2nd DCE.

@mraleph @mkustermann What do you think?

[alexmarkov]

This problem doesn't appear on stable. It seems like previously code generation of EqualityCompare handled boxed inputs even if instruction requested unboxed representation in RequiredInputRepresentation. That code was replaced with UNREACHABLE in the recent change https://dart-review.googlesource.com/c/sdk/+/185660.

[mraleph]

I feel that adding a pass like this is a wrong approach - because you can't guarantee that this sort of replacement does not happen later.

I think the right approach here is to track on the graph whether representations can be mismatched (i.e. whether we are expected to legalize the graph by running select represenations) and if they can't be mismatched (after the second SelectRepresentations) then optimisations should be prohibited from making changes to the graph which make it invalid.

This would mean that CheckedSmiComparison -> EqualityCompare replacement would have to insert unboxing itself.

[alexmarkov]

The 2nd SelectRepresentations pass is performed very late in the compilation pipeline. All transformations after that are supposed to maintain legal IL wrt representations. This is very fragile and easy to break.

Canonicalize performs peephole (local) transformations of IL instructions, so running the pass before SelectRepresentations would guarantee that there are no further opportunities for Canonicalize transformations afterwards unless further passes add such opportunities. Also note that the last TypePropagation happens before the last SelectRepresentations, so compiler shouldn't discover any new types after SelectRepresentations. This guarantees that any type-based transformations in Canonicalize pass (which includes CheckedSmiComparison -> EqualityCompare replacement) will not happen (but only if we insert an extra Canonicalize before SelectRepresentations). That's why I think this is the right approach.

On the other hand, adding custom unboxing in one particular case of Canonicalize transformation seems like duplicating what SelectRepresentations does, and also doesn't cover any other current and future type-based transformations in Canonicalize which might also change representation requirements and suffer from the same problem. We will also need to guard against some inputs being unboxed to avoid unnecessary Box/Unbox pairs added by SelectRepresentations/Canonicalize.

In order to make our passes more robust we can verify representations in flow graph checker. I filed https://github.com/dart-lang/sdk/issues/45509 for that.

[alexmarkov]

The test co19/Language/Functions/syntax_t03 starts passing after CheckedSmiComparison instruction was replaced with null-aware EqualityCompare in https://dart-review.googlesource.com/c/sdk/+/193447.

Theoretically it should be possible to recreate the same situation with null-aware EqualityCompare as it has similar canonicalization rule which converts it to the unboxed EqualityCompare. However, even after tweaking the test I wasn't able to reproduce the problem anymore.