IssueTracker

Thank you for the report and reproduction. I can reproduce on a Android TV emulator.

Status of current findings.

This is related to the SplitReturnRewriter added in https://r8.googlesource.com/r8/+/0c68ba1dc945da74c2f72085b401fe46635b3fc9.

Not running this code pass on java.lang.Object androidx.collection.LruCache.remove(java.lang.Object) works around the issue. I used the following patch:

diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/SplitReturnRewriter.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/SplitReturnRewriter.java
index 157d26c66a..63e0f76064 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/passes/SplitReturnRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/SplitReturnRewriter.java
@@ -43,6 +43,10 @@ public class SplitReturnRewriter extends CodeRewriterPass<AppInfo> {
 
   @Override
   protected CodeRewriterResult rewriteCode(IRCode code) {
+    if (code.context().toSourceString().equals(
+        "java.lang.Object androidx.collection.LruCache.remove(java.lang.Object)")) {
+      return CodeRewriterResult.NO_CHANGE;
+    }
     int color = colorExceptionHandlers(code);
     AffectedValues affectedValues = new AffectedValues();
     boolean changed = false;

Comparing the DEX with and without this patch shows one instruction diff in void androidx.leanback.widget.GridLayoutManager$2.addItem(java.lang.Object, int, int, int, int) where java.lang.Object androidx.collection.LruCache.remove(java.lang.Object) is inlined into.

Tracing the IR for void androidx.leanback.widget.GridLayoutManager$2.addItem(java.lang.Object, int, int, int, int) for the two versions shows that they start to deviate in second pass of IR after inserting assume instructions (SSA) with

block 29, pred-counts: 1, succ-count: 2, filled: true, sealed: false
predecessors: 28 
successors: 31 30  (no try/catch successors)
no phis
 #143:loadView;1762:addItem: -1: InstanceGet          v67 <- v136; field: androidx.collection.LruCache androidx.leanback.widget.ViewsStateBundle.mChildStates
                           : -1: Assume               v140 <- v67; lower bound: @Nullable androidx.collection.LruCache {}
                           : -1: SafeCheckCast        v131 <- v65; java.lang.String
                           : -1: Invoke-Virtual       v132 <- v140, v131; method: java.lang.Object androidx.collection.LruCache.remove(java.lang.String)
                           : -1: CheckCast            v69 <- v132; android.util.SparseArray
 #144:loadView;1762:addItem: -1: If                   v69 EQZ block 31 (fallthrough 30)

vs.

block 29, pred-counts: 1, succ-count: 2, filled: true, sealed: false
predecessors: 28 
successors: 31 30  (no try/catch successors)
no phis
 #143:loadView;1762:addItem: -1: InstanceGet          v67 <- v136; field: androidx.collection.LruCache androidx.leanback.widget.ViewsStateBundle.mChildStates
                           : -1: Assume               v140 <- v67; lower bound: @Nullable androidx.collection.LruCache {}
                           : -1: SafeCheckCast        v131 <- v65; java.lang.String
                           : -1: Invoke-Virtual       v132 <- v140, v131; method: java.lang.Object androidx.collection.LruCache.remove(java.lang.String)
                           : -1: Assume               v141 <- v132; not null
                           : -1: CheckCast            v69 <- v141; android.util.SparseArray
 #144:loadView;1762:addItem: -1: If                   v69 EQZ block 31 (fallthrough 30)

This is from computeDynamicReturnType concluding a dynamic return type of DynamicTypeWithUpperBound(@NotNull java.lang.Object {}).

Looks like dead code removal in java.lang.Object androidx.collection.LruCache.remove(java.lang.Object) is causing this.

The method java.lang.Object androidx.collection.LruCache.remove(java.lang.Object) is processed twice in first round of IR, second time with more precise type for the argument as java.lang.Object androidx.collection.LruCache.remove(java.lang.String).

That second processing involves quite a bit of inlining. However before dead code removal the return is still split between null and not null return value:

block 33, pred-counts: 1, succ-count: 2, filled: true, sealed: false
predecessors: 32 
successors: 37 34  (no try/catch successors)
no phis
 #187:remove;0:remove: -1: If                   v54 EQZ block 37 (fallthrough 34)

block 34, pred-counts: 1, succ-count: 1, filled: true, sealed: false
predecessors: 33 
successors: 35  (no try/catch successors)
no phis
 #187:remove;0:remove: -1: Assume               v49 <- v54; not null
 #188:remove;0:remove: -1: ConstNumber          v32(0b0) <-  0 (INT)
                     : -1: ConstNumber          v33(0) <-  0 (@Null NULL)
                     : -1: Goto                 block 35

block 35, pred-counts: 1, succ-count: 0, filled: true, sealed: false
predecessors: 34 
successors: -
no phis
 #188:remove;0:remove: -1: ConstString          v90 <-  "key"
                     : -1: Invoke-Static        v45, v90; method: void kotlin.jvm.internal.Intrinsics.checkNotNullParameter(java.lang.Object, java.lang.String)
                     : -1: ConstString          v91 <-  "oldValue"
                     : -1: Invoke-Static        v49, v91; method: void kotlin.jvm.internal.Intrinsics.checkNotNullParameter(java.lang.Object, java.lang.String)
 #191:remove;0:remove: -1: Return               v49

block 37, pred-counts: 1, succ-count: 0, filled: true, sealed: false
predecessors: 33 
successors: -
no phis
 #191:remove;0:remove: -1: Return               v54

After dead code removal a single return is left with the non null assumption:

block 34, pred-counts: 1, succ-count: 1, filled: true, sealed: false
predecessors: 32 
successors: 35  (no try/catch successors)
no phis
 #187:remove;0:remove: -1: Assume               v49 <- v54; not null
 #188:remove;0:remove: -1: Goto                 block 35

block 35, pred-counts: 1, succ-count: 0, filled: true, sealed: false
predecessors: 34 
successors: -
no phis
 #191:remove;0:remove: -1: Return               v49

For reference the source of the method is:

    public fun remove(key: K): V? {
        val previous: V?
        lock.synchronized {
            previous = map.remove(key)
            if (previous != null) {
                size -= safeSizeOf(key, previous)
            }
        }

        if (previous != null) {
            entryRemoved(false, key, previous, null)
        }

        return previous
    }

with the default implementation of entryRemoved doing nothing:

    protected open fun entryRemoved(evicted: Boolean, key: K, oldValue: V, newValue: V?) {}

The underlying issue is that branch simplification (as part of dead code removal) is comparing return values through their aliased values (in valuesAreIdentical) potentially conflating return values with different assume instructions. As this happens before collecting optimization info this can lead to the wrong dynamic type for the return value.

  private boolean valuesAreIdentical(Value value, Value other) {
    value = value.getAliasedValue();
    other = other.getAliasedValue();
    if (value == other) {
      return true;
    }
    if (value.isPhi() || other.isPhi()) {
      return false;
    }
    return instructionsDefineIdenticalValues(value.definition, other.definition);
  }

Project: r8
Branch: main
Author: Søren Gjesse <sgjesse@google.com>
Link:      https://r8-review.googlesource.com/101409

Add reproduction of b/395489597

Add reproduction of b/395489597 
 
Bug: b/395489597 
Change-Id: If769fae5db93ee8e2cfc1cf9feeddd847b00832b

Project: r8
Branch: main
Author: Søren Gjesse <sgjesse@google.com>
Link:      https://r8-review.googlesource.com/101480

Don't use aliased values when comparing values for basic block subsumption

Don't use aliased values when comparing values for basic block subsumption 
 
Aliased values can be defined by semantically different assume instructions  and then the resulting value chosen can have 
information which does not correctly represent both values. 
 
Fixes: b/395489597 
Change-Id: Ieaeb4fbf8d49cdc60d2912791cc2b701d42d2b34

Project: r8
Branch: 8.9
Author: Søren Gjesse <sgjesse@google.com>
Link:      https://r8-review.googlesource.com/101501

Version 8.9.27

Version 8.9.27 
 
R=christofferqa@google.com 
 
Bug: b/395489597 
Change-Id: I4a2483c145c93f9370e3af2c59bd3b2be487441d

Project: r8
Branch: 8.9
Author: Søren Gjesse <sgjesse@google.com>
Link:      https://r8-review.googlesource.com/101483

Don't use aliased values when comparing values for basic block subsumption

Don't use aliased values when comparing values for basic block subsumption 
 
Aliased values can be defined by semantically different assume instructions  and then the resulting value chosen can have 
information which does not correctly represent both values. 
 
R=christofferqa@google.com 
 
Fixes: b/395489597 
Change-Id: Ieaeb4fbf8d49cdc60d2912791cc2b701d42d2b34

Project: r8
Branch: 8.8
Author: Søren Gjesse <sgjesse@google.com>
Link:      https://r8-review.googlesource.com/101502

Don't use aliased values when comparing values for basic block subsumption

Don't use aliased values when comparing values for basic block subsumption 
 
Aliased values can be defined by semantically different assume instructions  and then the resulting value chosen can have 
information which does not correctly represent both values. 
 
R=christofferqa@google.com 
 
Fixes: b/395489597 
Change-Id: Ieaeb4fbf8d49cdc60d2912791cc2b701d42d2b34