Extend canonicalizer to constant, non-throwing fields
Bug: b/236648397
Bug: b/236661949
Bug: b/237372251
Change-Id: I7dc761ea342a30efb8b243678cd2424645ba9624
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
index b785927..51a4a4a 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedMethod.java
@@ -447,10 +447,6 @@
return isInstanceInitializer() || isClassInitializer();
}
- public boolean isInitializer(boolean isStatic) {
- return isStatic ? isClassInitializer() : isInstanceInitializer();
- }
-
public boolean isInstanceInitializer() {
checkIfObsolete();
return accessFlags.isConstructor() && !accessFlags.isStatic();
diff --git a/src/main/java/com/android/tools/r8/ir/code/FieldGet.java b/src/main/java/com/android/tools/r8/ir/code/FieldGet.java
index ec663fb..4d4a55b 100644
--- a/src/main/java/com/android/tools/r8/ir/code/FieldGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/FieldGet.java
@@ -13,6 +13,10 @@
TypeElement getOutType();
+ boolean isInstanceGet();
+
+ boolean isStaticGet();
+
boolean hasUsedOutValue();
Value outValue();
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
index b88555d..6f571d0 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
@@ -39,6 +39,25 @@
super(field, dest, object);
}
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ public static InstanceGet copyOf(IRCode code, InstanceGet original) {
+ Value newValue =
+ new Value(code.valueNumberGenerator.next(), original.getOutType(), original.getLocalInfo());
+ return copyOf(newValue, original);
+ }
+
+ public static InstanceGet copyOf(Value newValue, InstanceGet original) {
+ assert newValue != original.outValue();
+ return InstanceGet.builder()
+ .setField(original.getField())
+ .setObject(original.object())
+ .setOutValue(newValue)
+ .build();
+ }
+
@Override
public int opcode() {
return Opcodes.INSTANCE_GET;
@@ -234,4 +253,35 @@
public boolean instructionMayTriggerMethodInvocation(AppView<?> appView, ProgramMethod context) {
return false;
}
+
+ @Override
+ public boolean instructionTypeCanBeCanonicalized() {
+ return true;
+ }
+
+ public static class Builder extends BuilderBase<Builder, InstanceGet> {
+
+ private DexField field;
+ private Value object;
+
+ public Builder setField(DexField field) {
+ this.field = field;
+ return this;
+ }
+
+ public Builder setObject(Value object) {
+ this.object = object;
+ return this;
+ }
+
+ @Override
+ public InstanceGet build() {
+ return amend(new InstanceGet(outValue, object, field));
+ }
+
+ @Override
+ public Builder self() {
+ return this;
+ }
+ }
}
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 276a72c..06b2d1c 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -128,7 +128,6 @@
private final CfgPrinter printer;
public final CodeRewriter codeRewriter;
private final NaturalIntLoopRemover naturalIntLoopRemover = new NaturalIntLoopRemover();
- private final ConstantCanonicalizer constantCanonicalizer;
public final MemberValuePropagation<?> memberValuePropagation;
private final LensCodeRewriter lensCodeRewriter;
private final Inliner inliner;
@@ -176,7 +175,6 @@
this.options = appView.options();
this.printer = printer;
this.codeRewriter = new CodeRewriter(appView);
- this.constantCanonicalizer = new ConstantCanonicalizer(codeRewriter);
this.classInitializerDefaultsOptimization =
new ClassInitializerDefaultsOptimization(appView, this);
this.stringOptimizer = new StringOptimizer(appView);
@@ -1430,7 +1428,7 @@
// TODO(mkroghj) Test if shorten live ranges is worth it.
if (!options.isGeneratingClassFiles()) {
timing.begin("Canonicalize constants");
- constantCanonicalizer.canonicalize(appView, code);
+ new ConstantCanonicalizer(appView, codeRewriter, context, code).canonicalize();
timing.end();
previous = printMethod(code, "IR after constant canonicalization (SSA)", previous);
timing.begin("Create constants for literal instructions");
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/ConstantCanonicalizer.java b/src/main/java/com/android/tools/r8/ir/optimize/ConstantCanonicalizer.java
index 7082cc6..7a0a731 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/ConstantCanonicalizer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/ConstantCanonicalizer.java
@@ -7,13 +7,17 @@
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.DEX_ITEM_BASED_CONST_STRING;
+import static com.android.tools.r8.ir.code.Opcodes.INSTANCE_GET;
import static com.android.tools.r8.ir.code.Opcodes.STATIC_GET;
import com.android.tools.r8.errors.Unreachable;
+import com.android.tools.r8.graph.AppInfoWithClassHierarchy;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexClass;
-import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.graph.FieldAccessFlags;
+import com.android.tools.r8.graph.FieldResolutionResult.SingleFieldResolutionResult;
+import com.android.tools.r8.graph.ProgramField;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.ir.analysis.value.AbstractValue;
import com.android.tools.r8.ir.analysis.value.SingleFieldValue;
@@ -22,7 +26,10 @@
import com.android.tools.r8.ir.code.ConstNumber;
import com.android.tools.r8.ir.code.ConstString;
import com.android.tools.r8.ir.code.DexItemBasedConstString;
+import com.android.tools.r8.ir.code.FieldGet;
+import com.android.tools.r8.ir.code.FieldInstruction;
import com.android.tools.r8.ir.code.IRCode;
+import com.android.tools.r8.ir.code.InstanceGet;
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.StaticGet;
@@ -39,17 +46,44 @@
* Canonicalize constants.
*/
public class ConstantCanonicalizer {
+
// Threshold to limit the number of constant canonicalization.
private static final int MAX_CANONICALIZED_CONSTANT = 22;
+ private final AppView<?> appView;
private final CodeRewriter codeRewriter;
+ private final ProgramMethod context;
+ private final IRCode code;
+ private final boolean isAccessingVolatileField;
- public ConstantCanonicalizer(CodeRewriter codeRewriter) {
+ public ConstantCanonicalizer(
+ AppView<?> appView, CodeRewriter codeRewriter, ProgramMethod context, IRCode code) {
+ this.appView = appView;
this.codeRewriter = codeRewriter;
+ this.context = context;
+ this.code = code;
+ this.isAccessingVolatileField = computeIsAccessingVolatileField(appView, code);
}
- public void canonicalize(AppView<?> appView, IRCode code) {
- ProgramMethod context = code.context();
+ private static boolean computeIsAccessingVolatileField(AppView<?> appView, IRCode code) {
+ if (!appView.hasClassHierarchy()) {
+ // Conservatively return true.
+ return true;
+ }
+ AppInfoWithClassHierarchy appInfo = appView.appInfoWithClassHierarchy();
+ for (FieldInstruction fieldGet :
+ code.<FieldInstruction>instructions(Instruction::isFieldInstruction)) {
+ SingleFieldResolutionResult<?> resolutionResult =
+ appInfo.resolveField(fieldGet.getField()).asSingleFieldResolutionResult();
+ if (resolutionResult == null
+ || resolutionResult.getResolvedField().getAccessFlags().isVolatile()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public void canonicalize() {
Object2ObjectLinkedOpenCustomHashMap<Instruction, List<Value>> valuesDefinedByConstant =
new Object2ObjectLinkedOpenCustomHashMap<>(
new Strategy<Instruction>() {
@@ -67,8 +101,9 @@
return candidate.asConstString().getValue().hashCode();
case DEX_ITEM_BASED_CONST_STRING:
return candidate.asDexItemBasedConstString().getItem().hashCode();
+ case INSTANCE_GET:
case STATIC_GET:
- return candidate.asStaticGet().getField().hashCode();
+ return candidate.asFieldGet().getField().hashCode();
default:
throw new Unreachable();
}
@@ -76,71 +111,26 @@
@Override
public boolean equals(Instruction a, Instruction b) {
- // Constants with local info must not be canonicalized and must be filtered.
- assert a == null || !a.outValue().hasLocalInfo();
- assert b == null || !b.outValue().hasLocalInfo();
- return a == b || (a != null && b != null && a.identicalNonValueNonPositionParts(b));
+ if (a == b) {
+ return true;
+ }
+ if (a == null || b == null || a.getClass() != b.getClass()) {
+ return false;
+ }
+ if (a.isInstanceGet() && a.getFirstOperand() != b.getFirstOperand()) {
+ return false;
+ }
+ return a.identicalNonValueNonPositionParts(b);
}
});
// Collect usages of constants that can be canonicalized.
for (Instruction current : code.instructions()) {
- // Interested only in instructions types that can be canonicalized, i.e., ConstClass,
- // ConstNumber, (DexItemBased)?ConstString, and StaticGet.
- if (!current.instructionTypeCanBeCanonicalized()) {
- continue;
+ if (isConstantCanonicalizationCandidate(current)) {
+ List<Value> oldValuesDefinedByConstant =
+ valuesDefinedByConstant.computeIfAbsent(current, k -> new ArrayList<>());
+ oldValuesDefinedByConstant.add(current.outValue());
}
- // Do not canonicalize ConstClass that may have side effects. Its original instructions
- // will not be removed by dead code remover due to the side effects.
- if (current.isConstClass() && current.instructionMayHaveSideEffects(appView, context)) {
- continue;
- }
- // Do not canonicalize ConstString instructions if there are monitor operations in the code.
- // That could lead to unbalanced locking and could lead to situations where OOM exceptions
- // could leave a synchronized method without unlocking the monitor.
- if ((current.isConstString() || current.isDexItemBasedConstString())
- && code.metadata().mayHaveMonitorInstruction()) {
- continue;
- }
- // Canonicalize effectively final fields that are guaranteed to be written before they are
- // read. This is only OK if the instruction cannot have side effects.
- if (current.isStaticGet()) {
- AbstractValue abstractValue = current.outValue().getAbstractValue(appView, context);
- if (!abstractValue.isSingleFieldValue()) {
- continue;
- }
- SingleFieldValue singleFieldValue = abstractValue.asSingleFieldValue();
- DexType fieldHolderType = singleFieldValue.getField().getHolderType();
- if (context.getDefinition().isClassInitializer()
- && context.getHolderType() == fieldHolderType) {
- // Avoid that canonicalization inserts a read before the unique write in the class
- // initializer, as that would change the program behavior.
- continue;
- }
- DexClass fieldHolder = appView.definitionFor(fieldHolderType);
- DexEncodedField field = singleFieldValue.getField().lookupOnClass(fieldHolder);
- if (field == null
- || !field.isEnum()
- || current.instructionMayHaveSideEffects(appView, context)) {
- // Only allow canonicalization of enums.
- continue;
- }
- }
- // Constants with local info must not be canonicalized and must be filtered.
- if (current.outValue().hasLocalInfo()) {
- continue;
- }
- // Constants that are used by invoke range are not canonicalized to be compliant with the
- // optimization splitRangeInvokeConstant that gives the register allocator more freedom in
- // assigning register to ranged invokes which can greatly reduce the number of register
- // needed (and thereby code size as well). Thus no need to do a transformation that should
- // be removed later by another optimization.
- if (constantUsedByInvokeRange(current)) {
- continue;
- }
- List<Value> oldValuesDefinedByConstant =
- valuesDefinedByConstant.computeIfAbsent(current, k -> new ArrayList<>());
- oldValuesDefinedByConstant.add(current.outValue());
}
if (valuesDefinedByConstant.isEmpty()) {
@@ -189,13 +179,16 @@
DexItemBasedConstString.copyOf(
code, canonicalizedConstant.asDexItemBasedConstString());
break;
+ case INSTANCE_GET:
+ newConst = InstanceGet.copyOf(code, canonicalizedConstant.asInstanceGet());
+ break;
case STATIC_GET:
newConst = StaticGet.copyOf(code, canonicalizedConstant.asStaticGet());
break;
default:
throw new Unreachable();
}
- insertCanonicalizedConstant(code, newConst);
+ insertCanonicalizedConstant(newConst);
}
for (Value outValue : entry.getValue()) {
outValue.replaceUsers(newConst.outValue());
@@ -212,7 +205,152 @@
assert code.isConsistentSSA(appView);
}
- private static void insertCanonicalizedConstant(IRCode code, Instruction canonicalizedConstant) {
+ public boolean isConstantCanonicalizationCandidate(Instruction instruction) {
+ // Interested only in instructions types that can be canonicalized, i.e., ConstClass,
+ // ConstNumber, (DexItemBased)?ConstString, InstanceGet and StaticGet.
+ switch (instruction.opcode()) {
+ case CONST_CLASS:
+ // Do not canonicalize ConstClass that may have side effects. Its original instructions
+ // will not be removed by dead code remover due to the side effects.
+ if (instruction.instructionMayHaveSideEffects(appView, context)) {
+ return false;
+ }
+ break;
+ case CONST_NUMBER:
+ break;
+ case CONST_STRING:
+ case DEX_ITEM_BASED_CONST_STRING:
+ // Do not canonicalize ConstString instructions if there are monitor operations in the code.
+ // That could lead to unbalanced locking and could lead to situations where OOM exceptions
+ // could leave a synchronized method without unlocking the monitor.
+ if (code.metadata().mayHaveMonitorInstruction()) {
+ return false;
+ }
+ break;
+ case INSTANCE_GET:
+ {
+ InstanceGet instanceGet = instruction.asInstanceGet();
+ if (!instanceGet.object().isThis()) {
+ // TODO(b/236661949): Generalize this to more receivers. For canonicalization we need
+ // the receiver to be non-null (or the instruction can throw) and we also currently
+ // require the receiver to be defined on-entry, since we always hoist constants to the
+ // entry block.
+ return false;
+ }
+ if (!isReadOfFinalFieldOutsideInitializer(instanceGet)) {
+ return false;
+ }
+ break;
+ }
+ case STATIC_GET:
+ {
+ // Canonicalize effectively final fields that are guaranteed to be written before they are
+ // read. This is only OK if the instruction cannot have side effects.
+ StaticGet staticGet = instruction.asStaticGet();
+ if (staticGet.instructionMayHaveSideEffects(appView, context)) {
+ return false;
+ }
+ if (!isReadOfFinalFieldOutsideInitializer(staticGet)
+ && !isEffectivelyFinalField(staticGet)) {
+ return false;
+ }
+ break;
+ }
+ default:
+ assert !instruction.instructionTypeCanBeCanonicalized() : instruction.toString();
+ return false;
+ }
+ // Constants with local info must not be canonicalized and must be filtered.
+ if (instruction.outValue().hasLocalInfo()) {
+ return false;
+ }
+ // Constants that are used by invoke range are not canonicalized to be compliant with the
+ // optimization splitRangeInvokeConstant that gives the register allocator more freedom in
+ // assigning register to ranged invokes which can greatly reduce the number of register
+ // needed (and thereby code size as well). Thus no need to do a transformation that should
+ // be removed later by another optimization.
+ if (constantUsedByInvokeRange(instruction)) {
+ return false;
+ }
+ return true;
+ }
+
+ private boolean isReadOfFinalFieldOutsideInitializer(FieldGet fieldGet) {
+ if (isAccessingVolatileField) {
+ // A final field may be initialized concurrently. A requirement for this is that the field is
+ // volatile. However, the reading or writing of another volatile field also allows for
+ // concurrently initializing a non-volatile field. See also redundant field load elimination.
+ return false;
+ }
+ AppView<? extends AppInfoWithClassHierarchy> appViewWithClassHierarchy =
+ appView.withClassHierarchy();
+ SingleFieldResolutionResult<?> resolutionResult =
+ appViewWithClassHierarchy
+ .appInfo()
+ .resolveField(fieldGet.getField())
+ .asSingleFieldResolutionResult();
+ if (resolutionResult == null) {
+ // Not known to be final.
+ return false;
+ }
+ if (!resolutionResult.isSingleProgramFieldResolutionResult()) {
+ // We can't rely on the final flag of non-program fields.
+ return false;
+ }
+ ProgramField resolvedField = resolutionResult.getSingleProgramField();
+ FieldAccessFlags accessFlags = resolvedField.getAccessFlags();
+ assert !accessFlags.isVolatile();
+ // TODO(b/236661949): Add support for effectively final fields so that this also works well
+ // without -allowaccessmodification.
+ if (!accessFlags.isFinal()) {
+ return false;
+ }
+ if (appView.getKeepInfo(resolvedField).isPinned(appView.options())) {
+ // The final flag could be unset using reflection.
+ return false;
+ }
+ if (context.getDefinition().isInitializer()
+ && context.getAccessFlags().isStatic() == fieldGet.isStaticGet()) {
+ if (context.getHolder() == resolvedField.getHolder()) {
+ // If this is an initializer on the field's holder, then bail out, since the field value is
+ // only known to be final after object/class creation.
+ return false;
+ }
+ if (fieldGet.isInstanceGet()
+ && appViewWithClassHierarchy
+ .appInfo()
+ .isSubtype(context.getHolder(), resolvedField.getHolder())) {
+ // If an instance initializer reads a final instance field declared in a super class, we
+ // cannot hoist the read above the parent constructor call.
+ return false;
+ }
+ }
+ if (!resolutionResult.getInitialResolutionHolder().isResolvable(appView)) {
+ // If this field read is guarded by an API level check, hoisting of this field could lead to
+ // a ClassNotFoundException on some API levels.
+ return false;
+ }
+ return true;
+ }
+
+ private boolean isEffectivelyFinalField(StaticGet staticGet) {
+ AbstractValue abstractValue = staticGet.outValue().getAbstractValue(appView, context);
+ if (!abstractValue.isSingleFieldValue()) {
+ return false;
+ }
+ SingleFieldValue singleFieldValue = abstractValue.asSingleFieldValue();
+ DexType fieldHolderType = singleFieldValue.getField().getHolderType();
+ if (context.getDefinition().isClassInitializer()
+ && context.getHolderType() == fieldHolderType) {
+ // Avoid that canonicalization inserts a read before the unique write in the class
+ // initializer, as that would change the program behavior.
+ return false;
+ }
+ DexClass fieldHolder = appView.definitionFor(fieldHolderType);
+ return singleFieldValue.getField().lookupOnClass(fieldHolder) != null;
+ }
+
+ private void insertCanonicalizedConstant(Instruction canonicalizedConstant) {
BasicBlock entryBlock = code.entryBlock();
// Insert the constant instruction at the start of the block right after the argument
// instructions. It is important that the const instruction is put before any instruction
diff --git a/src/main/java/com/android/tools/r8/optimize/AccessModifier.java b/src/main/java/com/android/tools/r8/optimize/AccessModifier.java
index d8b5946..c1ee380 100644
--- a/src/main/java/com/android/tools/r8/optimize/AccessModifier.java
+++ b/src/main/java/com/android/tools/r8/optimize/AccessModifier.java
@@ -148,7 +148,8 @@
&& !accessInfo.isWrittenFromMethodHandle()
&& accessInfo.isWrittenOnlyInMethodSatisfying(
method ->
- method.getDefinition().isInitializer(flags.isStatic())
+ method.getDefinition().isInitializer()
+ && method.getAccessFlags().isStatic() == flags.isStatic()
&& method.getHolder() == field.getHolder())
&& !flags.isFinal()
&& !flags.isVolatile()) {
