Revert "Reland "Analyze written-before-read property at allocation sites"" This reverts commit 18d01e1b55289d810cc99d49081b02e8bc828835. Reason for revert: fails on platform, see https://android-build.corp.google.com/test_investigate/invocation/I04800010289154153/test/TR56429348958799735/?blocking=blocking&status=none Change-Id: I98e43dc5b7e149a25f1538028619599dbcd5c757
diff --git a/src/main/java/com/android/tools/r8/ir/code/Phi.java b/src/main/java/com/android/tools/r8/ir/code/Phi.java index 7bafe1b..a6f39aa 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Phi.java +++ b/src/main/java/com/android/tools/r8/ir/code/Phi.java
@@ -200,15 +200,6 @@ return operands; } - public boolean hasOperandThatMatches(Predicate<Value> predicate) { - for (Value operand : operands) { - if (predicate.test(operand)) { - return true; - } - } - return false; - } - public void removeOperand(int index) { removeOperand(index, null, alwaysFalse()); }
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java index e750309..80cf637 100644 --- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java +++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/DefaultFieldValueJoiner.java
@@ -7,8 +7,6 @@ import com.android.tools.r8.errors.Unreachable; import com.android.tools.r8.graph.AppView; -import com.android.tools.r8.graph.Code; -import com.android.tools.r8.graph.DefaultUseRegistryWithResult; import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.DexType; import com.android.tools.r8.graph.ProgramField; @@ -19,8 +17,6 @@ import com.android.tools.r8.ir.analysis.value.AbstractValue; import com.android.tools.r8.ir.analysis.value.AbstractValueFactory; import com.android.tools.r8.ir.code.IRCode; -import com.android.tools.r8.ir.code.Instruction; -import com.android.tools.r8.ir.code.NewInstance; import com.android.tools.r8.ir.conversion.MethodConversionOptions; import com.android.tools.r8.optimize.argumentpropagation.codescanner.ConcreteArrayTypeValueState; import com.android.tools.r8.optimize.argumentpropagation.codescanner.ConcreteClassTypeValueState; @@ -30,6 +26,7 @@ import com.android.tools.r8.optimize.argumentpropagation.codescanner.NonEmptyValueState; import com.android.tools.r8.optimize.argumentpropagation.codescanner.ValueState; import com.android.tools.r8.shaking.AppInfoWithLiveness; +import com.android.tools.r8.utils.IterableUtils; import com.android.tools.r8.utils.MapUtils; import com.android.tools.r8.utils.Pair; import com.android.tools.r8.utils.ThreadUtils; @@ -38,7 +35,6 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.Deque; import java.util.IdentityHashMap; import java.util.List; @@ -48,7 +44,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.function.Consumer; -import java.util.function.Predicate; public class DefaultFieldValueJoiner { @@ -74,9 +69,6 @@ // Find all the fields where we need to determine if each field read is guaranteed to be // dominated by a write. Map<DexProgramClass, List<ProgramField>> fieldsOfInterest = getFieldsOfInterest(); - if (fieldsOfInterest.isEmpty()) { - return Collections.emptyMap(); - } // If constructor inlining is disabled, then we focus on whether each instance initializer // definitely assigns the given field before it is read. We do the same for final and static @@ -102,12 +94,12 @@ // constructor inlining, we find all new-instance instructions (including subtype allocations) // and check if the field is written on each allocation before it is possibly read. analyzeNewInstanceInstructions( - fieldsNotSubjectToInitializerAnalysis, fieldsWithLiveDefaultValue::add, executorService); + fieldsNotSubjectToInitializerAnalysis, fieldsWithLiveDefaultValue::add); return updateFlowGraphs(fieldsWithLiveDefaultValue, executorService); } - protected Map<DexProgramClass, List<ProgramField>> getFieldsOfInterest() { + private Map<DexProgramClass, List<ProgramField>> getFieldsOfInterest() { Map<DexProgramClass, List<ProgramField>> fieldsOfInterest = new IdentityHashMap<>(); for (DexProgramClass clazz : appView.appInfo().classes()) { clazz.forEachProgramField( @@ -248,95 +240,13 @@ private void analyzeNewInstanceInstructions( Map<DexType, ProgramFieldSet> nonFinalInstanceFields, - Consumer<ProgramField> liveDefaultValueConsumer, - ExecutorService executorService) - throws ExecutionException { - // To simplify the analysis, we currently bail out for non-final classes. - // TODO(b/296030319): Handle non-final classes. - MapUtils.removeIf( - nonFinalInstanceFields, - (holderType, fields) -> { - assert !fields.isEmpty(); - DexProgramClass holder = fields.iterator().next().getHolder(); - if (holder.isFinal() || !appView.appInfo().isInstantiatedIndirectly(holder)) { - // When the class is not explicitly marked final, the class could in principle have - // injected subclasses if it is pinned. However, none of the fields are pinned, so we - // should be allowed to reason about the field assignments in the program. - assert fields.stream() - .allMatch( - field -> appView.getKeepInfo(field).isValuePropagationAllowed(appView, field)); - return false; - } - fields.forEach(liveDefaultValueConsumer); - return true; - }); - - // We analyze all allocations of the classes that declare one of the given fields. - ThreadUtils.processMethods( - appView, - method -> - analyzeNewInstanceInstructionsInMethod( - nonFinalInstanceFields, liveDefaultValueConsumer, method), - appView.options().getThreadingModule(), - executorService); - } - - private void analyzeNewInstanceInstructionsInMethod( - Map<DexType, ProgramFieldSet> nonFinalInstanceFields, - Consumer<ProgramField> liveDefaultValueConsumer, - ProgramMethod method) { - if (!maybeHasNewInstanceThatMatches(method, nonFinalInstanceFields::containsKey)) { - return; + Consumer<ProgramField> liveDefaultValueConsumer) { + // Conservatively treat all fields as maybe read before written. + // TODO(b/296030319): Implement analysis by building IR for all methods that instantiate the + // relevant classes and analyzing the puts to the newly created instances. + for (ProgramField field : IterableUtils.flatten(nonFinalInstanceFields.values())) { + liveDefaultValueConsumer.accept(field); } - IRCode code = method.buildIR(appView, MethodConversionOptions.nonConverting()); - for (NewInstance newInstance : code.<NewInstance>instructions(Instruction::isNewInstance)) { - ProgramFieldSet fieldsOfInterest = nonFinalInstanceFields.get(newInstance.getType()); - if (fieldsOfInterest == null) { - continue; - } - FieldReadBeforeWriteDfsAnalysis analysis = - new FieldReadBeforeWriteDfsAnalysis(appView, code, fieldsOfInterest, newInstance) { - - @Override - public AnalysisContinuation acceptFieldMaybeReadBeforeWrite(ProgramField field) { - // Remove this field from the `fieldsOfInterest`, so that we do not spend more time - // analyzing it. - if (fieldsOfInterest.remove(field)) { - liveDefaultValueConsumer.accept(field); - } - return AnalysisContinuation.abortIf(fieldsOfInterest.isEmpty()); - } - }; - analysis.run(); - if (fieldsOfInterest.isEmpty()) { - nonFinalInstanceFields.remove(newInstance.getType()); - } - } - } - - private boolean maybeHasNewInstanceThatMatches( - ProgramMethod method, Predicate<DexType> predicate) { - Code code = method.getDefinition().getCode(); - if (code == null || code.isSharedCodeObject()) { - return false; - } - if (code.isLirCode()) { - return code.asLirCode() - .hasConstantItemThatMatches( - constant -> constant instanceof DexType && predicate.test((DexType) constant)); - } - assert appView.isCfByteCodePassThrough(method); - assert code.isCfCode(); - return method.registerCodeReferencesWithResult( - new DefaultUseRegistryWithResult<>(appView, method, false) { - - @Override - public void registerNewInstance(DexType type) { - if (predicate.test(type)) { - setResult(true); - } - } - }); } private Map<FlowGraph, Deque<FlowGraphNode>> updateFlowGraphs(
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysis.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysis.java deleted file mode 100644 index 1fdfb27..0000000 --- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysis.java +++ /dev/null
@@ -1,387 +0,0 @@ -// Copyright (c) 2024, the R8 project authors. Please see the AUTHORS file -// for details. All rights reserved. Use of this source code is governed by a -// BSD-style license that can be found in the LICENSE file. -package com.android.tools.r8.optimize.argumentpropagation.propagation; - -import static com.android.tools.r8.ir.code.Opcodes.ASSUME; -import static com.android.tools.r8.ir.code.Opcodes.CHECK_CAST; -import static com.android.tools.r8.ir.code.Opcodes.CONST_NUMBER; -import static com.android.tools.r8.ir.code.Opcodes.CONST_STRING; -import static com.android.tools.r8.ir.code.Opcodes.GOTO; -import static com.android.tools.r8.ir.code.Opcodes.INSTANCE_PUT; -import static com.android.tools.r8.ir.code.Opcodes.INVOKE_DIRECT; -import static com.android.tools.r8.ir.code.Opcodes.INVOKE_INTERFACE; -import static com.android.tools.r8.ir.code.Opcodes.INVOKE_STATIC; -import static com.android.tools.r8.ir.code.Opcodes.INVOKE_SUPER; -import static com.android.tools.r8.ir.code.Opcodes.INVOKE_VIRTUAL; -import static com.android.tools.r8.ir.code.Opcodes.RETURN; -import static com.android.tools.r8.ir.code.Opcodes.THROW; - -import com.android.tools.r8.graph.AppView; -import com.android.tools.r8.graph.DexClassAndMethod; -import com.android.tools.r8.graph.DexItemFactory; -import com.android.tools.r8.graph.ProgramField; -import com.android.tools.r8.ir.code.Assume; -import com.android.tools.r8.ir.code.BasicBlock; -import com.android.tools.r8.ir.code.BasicBlockInstructionIterator; -import com.android.tools.r8.ir.code.CheckCast; -import com.android.tools.r8.ir.code.ConstNumber; -import com.android.tools.r8.ir.code.ConstString; -import com.android.tools.r8.ir.code.Goto; -import com.android.tools.r8.ir.code.IRCode; -import com.android.tools.r8.ir.code.InstancePut; -import com.android.tools.r8.ir.code.Instruction; -import com.android.tools.r8.ir.code.InvokeDirect; -import com.android.tools.r8.ir.code.InvokeMethod; -import com.android.tools.r8.ir.code.NewInstance; -import com.android.tools.r8.ir.code.Phi; -import com.android.tools.r8.ir.code.Return; -import com.android.tools.r8.ir.code.Throw; -import com.android.tools.r8.shaking.AppInfoWithLiveness; -import com.android.tools.r8.utils.TraversalContinuation; -import com.android.tools.r8.utils.WorkList; -import com.android.tools.r8.utils.collections.ProgramFieldSet; - -/** - * Analysis that is given an allocation site (a {@link NewInstance} instruction) and a set of fields - * that belong to that newly allocated instance. - * - * <p>The analysis computes the subset of the given fields which are maybe read before they are - * written. The default value of these fields is potentially read, whereas the default value of the - * complement field set are guaranteed to never be read. - * - * <p>The analysis works by exploring all possible paths starting from the given allocation site to - * the normal and the exceptional exits of the method, keeping track of which fields are definitely - * written before they are read and which fields have maybe been read. - */ -public abstract class FieldReadBeforeWriteDfsAnalysis extends FieldReadBeforeWriteDfsAnalysisState { - - private final AppView<AppInfoWithLiveness> appView; - private final IRCode code; - private final DexItemFactory dexItemFactory; - // The set of fields to consider. Note that this is a concurrent set and that the caller may - // concurrently remove fields from the set. This may happen if we concurrently find a - // read-before-write of one of the fields. - private final ProgramFieldSet fields; - private final WorkList<WorkItem> worklist = WorkList.newIdentityWorkList(); - - private final FieldReadBeforeWriteDfsAnalysis self = this; - - public FieldReadBeforeWriteDfsAnalysis( - AppView<AppInfoWithLiveness> appView, - IRCode code, - ProgramFieldSet fields, - NewInstance newInstance) { - super(newInstance); - this.appView = appView; - this.code = code; - this.dexItemFactory = appView.dexItemFactory(); - this.fields = fields; - } - - // Returns ABORT if all fields of interest are now maybe-read-before-written. - // Otherwise returns CONTINUE. - public abstract AnalysisContinuation acceptFieldMaybeReadBeforeWrite(ProgramField field); - - public void run() { - worklist.addIfNotSeen(new InitialWorkItem()); - worklist.run(WorkItem::process); - } - - public enum AnalysisContinuation { - // Signals to abort the analysis completely (i.e., to break out of the DFS). This is used when - // we've reported all fields as being maybe read before written. - ABORT, - // Signals to continue the current DFS. - CONTINUE, - // Signals that all fields have been written before they are read on the current program path, - // meaning that the algorithm does not need to explore any further. The algorithm should instead - // backtrack and explore any other program paths. - BREAK; - - static AnalysisContinuation abortIf(boolean condition) { - if (condition) { - return ABORT; - } - return CONTINUE; - } - - boolean isAbort() { - return this == ABORT; - } - - boolean isAbortOrContinue() { - return isAbort() || isContinue(); - } - - boolean isBreak() { - return this == BREAK; - } - - boolean isContinue() { - return this == CONTINUE; - } - - TraversalContinuation<?, ?> toTraversalContinuation() { - assert isAbortOrContinue(); - return TraversalContinuation.breakIf(isAbort()); - } - } - - abstract class WorkItem { - - abstract TraversalContinuation<?, ?> process(); - - void applyPhis(BasicBlock block) { - // TODO(b/339210038): When adding support for non-linear control flow, we need to implement - // backtracking of this (i.e., we should remove the out value from the object aliases again). - for (Phi phi : block.getPhis()) { - if (phi.hasOperandThatMatches(self::isMaybeInstance)) { - addInstanceAlias(phi); - } - } - } - - AnalysisContinuation applyInstructions(BasicBlockInstructionIterator instructionIterator) { - while (instructionIterator.hasNext()) { - Instruction instruction = instructionIterator.next(); - assert !instruction.hasOutValue() || !isMaybeInstance(instruction.outValue()); - AnalysisContinuation continuation; - // TODO(b/339210038): Extend this to many other instructions, such as ConstClass, - // InstanceOf, *Binop, etc. - switch (instruction.opcode()) { - case ASSUME: - continuation = applyAssumeInstruction(instruction.asAssume()); - break; - case CHECK_CAST: - continuation = applyCheckCastInstruction(instruction.asCheckCast()); - break; - case CONST_NUMBER: - continuation = applyConstNumber(instruction.asConstNumber()); - break; - case CONST_STRING: - continuation = applyConstString(instruction.asConstString()); - break; - case GOTO: - continuation = applyGotoInstruction(instruction.asGoto()); - break; - case INSTANCE_PUT: - continuation = applyInstancePut(instruction.asInstancePut()); - break; - case INVOKE_DIRECT: - case INVOKE_INTERFACE: - case INVOKE_STATIC: - case INVOKE_SUPER: - case INVOKE_VIRTUAL: - continuation = applyInvokeMethodInstruction(instruction.asInvokeMethod()); - break; - case RETURN: - continuation = applyReturnInstruction(instruction.asReturn()); - break; - case THROW: - continuation = applyThrowInstruction(instruction.asThrow()); - break; - default: - continuation = applyUnhandledInstruction(); - break; - } - if (continuation.isAbort()) { - return continuation; - } - if (continuation.isBreak()) { - break; - } - } - return AnalysisContinuation.CONTINUE; - } - - // TODO(b/339210038): When adding support for non-linear control flow, we need to implement - // backtracking of this (i.e., we should remove the out value from the object aliases again). - private AnalysisContinuation applyAssumeInstruction(Assume assume) { - if (isMaybeInstance(assume.src())) { - addInstanceAlias(assume.outValue()); - } - return AnalysisContinuation.CONTINUE; - } - - // TODO(b/339210038): When adding support for non-linear control flow, we need to implement - // backtracking of this (i.e., we should remove the out value from the object aliases again). - private AnalysisContinuation applyCheckCastInstruction(CheckCast checkCast) { - if (isMaybeInstance(checkCast.object())) { - addInstanceAlias(checkCast.outValue()); - } - // If the instance has escaped to the heap and this check-cast instruction throws, then it is - // possible that the instance is retrieved from the heap and all fields are read. - return markRemainingFieldsAsMaybeReadBeforeWrittenIfInstanceIsEscaped(); - } - - private AnalysisContinuation applyConstNumber(ConstNumber unusedConstNumber) { - return AnalysisContinuation.CONTINUE; - } - - private AnalysisContinuation applyConstString(ConstString unusedConstString) { - return AnalysisContinuation.CONTINUE; - } - - private AnalysisContinuation applyGotoInstruction(Goto gotoInstruction) { - BasicBlock targetBlock = gotoInstruction.getTarget(); - if (isBlockOnStack(targetBlock)) { - // Bail out in case of cycles. - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } else { - // Continue exploration into the successor block. - worklist.addIgnoringSeenSet(new ProcessBlockWorkItem(targetBlock)); - return AnalysisContinuation.CONTINUE; - } - } - - private AnalysisContinuation applyInstancePut(InstancePut instancePut) { - // If the instance has escaped and this instance-put instruction can throw, then the program - // can get the instance from the heap and read any field. Give up in this case. - if (isEscaped() && instancePut.instructionInstanceCanThrow(appView, code.context())) { - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } - - // Record if this is a definite write to one of the fields of interest. - if (isDefinitelyInstance(instancePut.object())) { - ProgramField resolvedField = - instancePut.resolveField(appView, code.context()).getProgramField(); - if (resolvedField != null && fields.contains(resolvedField)) { - addWrittenBeforeRead(resolvedField); - } - - // If all fields of interest are written before read, then stop the exploration of the - // current program path (but continue to explore any program paths from previous unexplored - // branches). - if (fields.allMatch(self::isWrittenBeforeRead)) { - return AnalysisContinuation.BREAK; - } - } - - // Record if the instance has escaped as a result of this instance-put. - if (!isEscaped() && isMaybeInstance(instancePut.value())) { - setEscaped(instancePut); - } - return AnalysisContinuation.CONTINUE; - } - - private AnalysisContinuation applyInvokeMethodInstruction(InvokeMethod invoke) { - // Allow calls to java.lang.Object.<init>(). - // TODO(b/339210038): Generalize this to other constructors. - if (invoke.isInvokeConstructor(dexItemFactory) - && isDefinitelyInstance(invoke.getFirstArgument())) { - DexClassAndMethod resolvedMethod = - invoke.resolveMethod(appView, code.context()).getResolutionPair(); - if (resolvedMethod != null - && resolvedMethod - .getReference() - .isIdenticalTo(dexItemFactory.objectMembers.constructor)) { - return AnalysisContinuation.CONTINUE; - } - } - - // Conservatively treat calls as reading any field if the receiver has escaped or is escaping. - if (!isEscaped() - && invoke.hasInValueThatMatches(self::isMaybeInstance) - && invoke.instructionMayHaveSideEffects(appView, code.context())) { - setEscaped(invoke); - } - - if (isEscaped()) { - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } - - // Otherwise, this is a call to a method where none of the arguments is an alias of the - // instance, and the instance has not escaped. Therefore, this call cannot read any of fields - // from the instance. - return AnalysisContinuation.CONTINUE; - } - - private AnalysisContinuation applyReturnInstruction(Return unusedReturnInstruction) { - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } - - private AnalysisContinuation applyThrowInstruction(Throw unusedThrowInstruction) { - return markRemainingFieldsAsMaybeReadBeforeWrittenIfInstanceIsEscaped(); - } - - private AnalysisContinuation applyUnhandledInstruction() { - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } - - AnalysisContinuation markRemainingFieldsAsMaybeReadBeforeWritten() { - for (ProgramField field : fields) { - if (!isWrittenBeforeRead(field)) { - AnalysisContinuation continuation = acceptFieldMaybeReadBeforeWrite(field); - assert continuation.isAbortOrContinue(); - if (continuation.isAbort()) { - return continuation; - } - } - } - // At this point we could also CONTINUE, but we check if the fields of interest have become - // empty as a result of concurrent modification. - return AnalysisContinuation.abortIf(fields.isEmpty()); - } - - AnalysisContinuation markRemainingFieldsAsMaybeReadBeforeWrittenIfInstanceIsEscaped() { - if (isEscaped()) { - return markRemainingFieldsAsMaybeReadBeforeWritten(); - } - return AnalysisContinuation.CONTINUE; - } - } - - class InitialWorkItem extends WorkItem { - - @Override - TraversalContinuation<?, ?> process() { - // We start the analysis from the unique constructor invoke instead of from the NewInstance - // instruction, since no instructions before the constructor call can read any fields from the - // uninitialized this. - // TODO(b/339210038): In principle it may be possible for the NewInstance value to flow into a - // phi before the unique constructor invoke. If this happens we would not record the phi as - // an alias when starting the analysis from the invoke-direct. - InvokeDirect uniqueConstructorInvoke = - getNewInstance().getUniqueConstructorInvoke(dexItemFactory); - if (uniqueConstructorInvoke == null) { - return markRemainingFieldsAsMaybeReadBeforeWritten().toTraversalContinuation(); - } - BasicBlock block = uniqueConstructorInvoke.getBlock(); - // TODO(b/339210038): Maybe allow exceptional control flow. - if (block.hasCatchHandlers()) { - return markRemainingFieldsAsMaybeReadBeforeWritten().toTraversalContinuation(); - } - addBlockToStack(block); - addInstanceAlias(getNewInstance().outValue()); - BasicBlockInstructionIterator instructionIterator = block.iterator(uniqueConstructorInvoke); - // Start the analysis from the invoke-direct instruction. This is important if we can tell - // that the constructor definitely writes some fields. - instructionIterator.previous(); - return applyInstructions(instructionIterator).toTraversalContinuation(); - } - } - - class ProcessBlockWorkItem extends WorkItem { - - private final BasicBlock block; - - ProcessBlockWorkItem(BasicBlock block) { - this.block = block; - } - - @Override - TraversalContinuation<?, ?> process() { - // TODO(b/339210038): Maybe allow exceptional control flow. - if (block.hasCatchHandlers()) { - return TraversalContinuation.breakIf( - markRemainingFieldsAsMaybeReadBeforeWritten().isAbort()); - } - addBlockToStack(block); - applyPhis(block); - AnalysisContinuation continuation = applyInstructions(block.iterator()); - assert continuation.isAbortOrContinue(); - return TraversalContinuation.breakIf(continuation.isAbort()); - } - } -}
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysisState.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysisState.java deleted file mode 100644 index 7e24c03..0000000 --- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/FieldReadBeforeWriteDfsAnalysisState.java +++ /dev/null
@@ -1,84 +0,0 @@ -// Copyright (c) 2024, the R8 project authors. Please see the AUTHORS file -// for details. All rights reserved. Use of this source code is governed by a -// BSD-style license that can be found in the LICENSE file. -package com.android.tools.r8.optimize.argumentpropagation.propagation; - -import com.android.tools.r8.graph.ProgramField; -import com.android.tools.r8.ir.code.BasicBlock; -import com.android.tools.r8.ir.code.Instruction; -import com.android.tools.r8.ir.code.NewInstance; -import com.android.tools.r8.ir.code.Value; -import com.android.tools.r8.utils.collections.ProgramFieldSet; -import com.google.common.collect.Sets; -import java.util.Set; - -/** - * The state we track during the field-maybe-read-before-write/field-never-read-before-written - * analysis. - */ -public class FieldReadBeforeWriteDfsAnalysisState { - - // The current allocation we are analyzing. - private final NewInstance newInstance; - - // The first instruction on the current program path starting from the `newInstance` instruction - // from which the `newInstance` value escapes. - private Instruction escape = null; - - // The set of values that *may* be aliases of the `newInstance` value. - private final Set<Value> instanceAliases = Sets.newIdentityHashSet(); - - // The set of blocks on the current program path. - private final Set<BasicBlock> stack = Sets.newIdentityHashSet(); - - // The set of fields that are guaranteed to be written before they are read on the current program - // path. - private final ProgramFieldSet writtenBeforeRead = ProgramFieldSet.create(); - - FieldReadBeforeWriteDfsAnalysisState(NewInstance newInstance) { - this.newInstance = newInstance; - } - - void addInstanceAlias(Value instanceAlias) { - boolean changed = instanceAliases.add(instanceAlias); - assert changed; - } - - void addBlockToStack(BasicBlock block) { - boolean changed = stack.add(block); - assert changed; - } - - void addWrittenBeforeRead(ProgramField field) { - writtenBeforeRead.add(field); - } - - NewInstance getNewInstance() { - return newInstance; - } - - boolean isBlockOnStack(BasicBlock block) { - return stack.contains(block); - } - - boolean isEscaped() { - return escape != null; - } - - boolean isDefinitelyInstance(Value value) { - return value.getAliasedValue() == newInstance.outValue(); - } - - boolean isMaybeInstance(Value value) { - return instanceAliases.contains(value); - } - - boolean isWrittenBeforeRead(ProgramField field) { - return writtenBeforeRead.contains(field); - } - - void setEscaped(Instruction escape) { - assert !isEscaped(); - this.escape = escape; - } -}
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java index 81e3424..d62328d 100644 --- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java +++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InFlowPropagator.java
@@ -46,7 +46,7 @@ final AppView<AppInfoWithLiveness> appView; final Set<DexProgramClass> classesWithSingleCallerInlinedInstanceInitializers; final IRConverter converter; - protected final FieldStateCollection fieldStates; + final FieldStateCollection fieldStates; final MethodStateCollectionByReference methodStates; public InFlowPropagator( @@ -113,15 +113,12 @@ private Map<FlowGraph, Deque<FlowGraphNode>> includeDefaultValuesInFieldStates( List<FlowGraph> flowGraphs, ExecutorService executorService) throws ExecutionException { - DefaultFieldValueJoiner joiner = createDefaultFieldValueJoiner(flowGraphs); + DefaultFieldValueJoiner joiner = + new DefaultFieldValueJoiner( + appView, classesWithSingleCallerInlinedInstanceInitializers, fieldStates, flowGraphs); return joiner.joinDefaultFieldValuesForFieldsWithReadBeforeWrite(executorService); } - protected DefaultFieldValueJoiner createDefaultFieldValueJoiner(List<FlowGraph> flowGraphs) { - return new DefaultFieldValueJoiner( - appView, classesWithSingleCallerInlinedInstanceInitializers, fieldStates, flowGraphs); - } - private void processFlowGraphs(List<FlowGraph> flowGraphs, ExecutorService executorService) throws ExecutionException { ThreadUtils.processItems(
diff --git a/src/main/java/com/android/tools/r8/optimize/compose/ComposeMethodProcessor.java b/src/main/java/com/android/tools/r8/optimize/compose/ComposeMethodProcessor.java index 22ed377..8d7498c 100644 --- a/src/main/java/com/android/tools/r8/optimize/compose/ComposeMethodProcessor.java +++ b/src/main/java/com/android/tools/r8/optimize/compose/ComposeMethodProcessor.java
@@ -9,7 +9,6 @@ import com.android.tools.r8.errors.Unreachable; import com.android.tools.r8.graph.AppView; import com.android.tools.r8.graph.DexMethod; -import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.ProgramField; import com.android.tools.r8.graph.ProgramMethod; import com.android.tools.r8.ir.analysis.constant.SparseConditionalConstantPropagation; @@ -34,8 +33,6 @@ import com.android.tools.r8.optimize.argumentpropagation.codescanner.MethodState; import com.android.tools.r8.optimize.argumentpropagation.codescanner.MethodStateCollectionByReference; import com.android.tools.r8.optimize.argumentpropagation.codescanner.ValueState; -import com.android.tools.r8.optimize.argumentpropagation.propagation.DefaultFieldValueJoiner; -import com.android.tools.r8.optimize.argumentpropagation.propagation.FlowGraph; import com.android.tools.r8.optimize.argumentpropagation.propagation.InFlowPropagator; import com.android.tools.r8.shaking.AppInfoWithLiveness; import com.android.tools.r8.utils.IterableUtils; @@ -45,8 +42,6 @@ import com.android.tools.r8.utils.collections.ProgramMethodSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; -import java.util.Collections; -import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -97,22 +92,7 @@ InFlowPropagator inFlowPropagator = new InFlowPropagator( - appView, null, converter, codeScanner.getFieldStates(), codeScanner.getMethodStates()) { - - @Override - protected DefaultFieldValueJoiner createDefaultFieldValueJoiner( - List<FlowGraph> flowGraphs) { - return new DefaultFieldValueJoiner(appView, null, fieldStates, flowGraphs) { - - @Override - protected Map<DexProgramClass, List<ProgramField>> getFieldsOfInterest() { - // We do not rely on the optimization of any fields in the Composable optimization - // pass. - return Collections.emptyMap(); - } - }; - } - }; + appView, null, converter, codeScanner.getFieldStates(), codeScanner.getMethodStates()); inFlowPropagator.run(executorService); ArgumentPropagatorOptimizationInfoPopulator optimizationInfoPopulator =
diff --git a/src/main/java/com/android/tools/r8/utils/collections/ProgramFieldSet.java b/src/main/java/com/android/tools/r8/utils/collections/ProgramFieldSet.java index 891593a..c778df3 100644 --- a/src/main/java/com/android/tools/r8/utils/collections/ProgramFieldSet.java +++ b/src/main/java/com/android/tools/r8/utils/collections/ProgramFieldSet.java
@@ -8,15 +8,14 @@ import com.android.tools.r8.graph.DexField; import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.ProgramField; -import com.google.common.base.Predicate; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import java.util.IdentityHashMap; import java.util.Iterator; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; import java.util.stream.Stream; public class ProgramFieldSet implements Iterable<ProgramField> { @@ -55,10 +54,6 @@ backing.putAll(fields.backing); } - public boolean allMatch(Predicate<? super ProgramField> predicate) { - return Iterables.all(this, predicate); - } - public boolean createAndAdd(DexProgramClass clazz, DexEncodedField definition) { return add(new ProgramField(clazz, definition)); }
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/PackagePrivateMembersAccessedTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/PackagePrivateMembersAccessedTest.java index fa46f4d..0d912b3 100644 --- a/src/test/java/com/android/tools/r8/classmerging/horizontal/PackagePrivateMembersAccessedTest.java +++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/PackagePrivateMembersAccessedTest.java
@@ -4,7 +4,6 @@ package com.android.tools.r8.classmerging.horizontal; -import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsentIf; import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; import static org.hamcrest.MatcherAssert.assertThat; @@ -36,9 +35,7 @@ .assertSuccessWithOutputLines("foo", "hello", "5", "foobar") .inspect( codeInspector -> { - assertThat( - codeInspector.clazz(C.class), - isAbsentIf(parameters.canInitNewInstanceUsingSuperclassConstructor())); + assertThat(codeInspector.clazz(C.class), isPresent()); assertThat(codeInspector.clazz(D.class), isPresent()); assertThat(codeInspector.clazz(E.class), isPresent()); });
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldWithConstructorInliningTest.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldWithConstructorInliningTest.java index 142d565..f952fa9 100644 --- a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldWithConstructorInliningTest.java +++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldWithConstructorInliningTest.java
@@ -3,10 +3,10 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8.ir.optimize.membervaluepropagation; -import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent; import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; import com.android.tools.r8.NeverClassInline; import com.android.tools.r8.TestBase; @@ -42,11 +42,17 @@ .inspect( inspector -> { ClassSubject aClassSubject = inspector.clazz(A.class); - assertThat(aClassSubject, isAbsent()); + // TODO(b/339210038): Should always be absent. + assertThat( + aClassSubject, + isPresentIf(parameters.canInitNewInstanceUsingSuperclassConstructor())); MethodSubject mainMethodSubject = inspector.clazz(Main.class).mainMethod(); assertThat(mainMethodSubject, isPresent()); - assertTrue(mainMethodSubject.streamInstructions().anyMatch(i -> i.isConstNumber(42))); + // TODO(b/339210038): Should always contain 42. + assertEquals( + parameters.canInitNewInstanceUsingSuperclassConstructor(), + mainMethodSubject.streamInstructions().noneMatch(i -> i.isConstNumber(42))); }) .run(parameters.getRuntime(), Main.class) .assertSuccessWithOutputLines("42");
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/fields/FieldInitializedByConstantArgumentMultipleConstructorsTest.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/fields/FieldInitializedByConstantArgumentMultipleConstructorsTest.java index 862fbc9..fca4473 100644 --- a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/fields/FieldInitializedByConstantArgumentMultipleConstructorsTest.java +++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/fields/FieldInitializedByConstantArgumentMultipleConstructorsTest.java
@@ -4,8 +4,8 @@ package com.android.tools.r8.ir.optimize.membervaluepropagation.fields; -import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent; import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent; +import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf; import static org.hamcrest.MatcherAssert.assertThat; import com.android.tools.r8.NeverClassInline; @@ -18,20 +18,21 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameter; -import org.junit.runners.Parameterized.Parameters; @RunWith(Parameterized.class) public class FieldInitializedByConstantArgumentMultipleConstructorsTest extends TestBase { - @Parameter(0) - public TestParameters parameters; + private final TestParameters parameters; - @Parameters(name = "{0}") + @Parameterized.Parameters(name = "{0}") public static TestParametersCollection data() { return getTestParameters().withAllRuntimesAndApiLevels().build(); } + public FieldInitializedByConstantArgumentMultipleConstructorsTest(TestParameters parameters) { + this.parameters = parameters; + } + @Test public void test() throws Exception { testForR8(parameters.getBackend()) @@ -50,7 +51,10 @@ ClassSubject testClassSubject = inspector.clazz(TestClass.class); assertThat(testClassSubject, isPresent()); assertThat(testClassSubject.uniqueMethodWithOriginalName("live"), isPresent()); - assertThat(testClassSubject.uniqueMethodWithOriginalName("dead"), isAbsent()); + // TODO(b/280275115): Constructor inlining regresses instance field value analysis. + assertThat( + testClassSubject.uniqueMethodWithOriginalName("dead"), + isPresentIf(parameters.canInitNewInstanceUsingSuperclassConstructor())); } static class TestClass {