Fix Swap issues with desugared library conversions
- Remove swap when unboxing and use a temp
- Don't swap when last but one parameter needs to
be converted and last parameter is wide.
Bug: b/266741933
Change-Id: I9b1aa35446540c1c2f382d41543c1173fbb62a52
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryAPIConverter.java b/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryAPIConverter.java
index 2d72b93..757b75f 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryAPIConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryAPIConverter.java
@@ -88,6 +88,7 @@
return rewriteLibraryInvoke(
instruction.asInvoke(),
methodProcessingContext,
+ freshLocalProvider,
localStackAllocator,
eventConsumer,
context);
@@ -237,6 +238,7 @@
private Collection<CfInstruction> rewriteLibraryInvoke(
CfInvoke invoke,
MethodProcessingContext methodProcessingContext,
+ FreshLocalProvider freshLocalProvider,
LocalStackAllocator localStackAllocator,
CfInstructionDesugaringEventConsumer eventConsumer,
ProgramMethod context) {
@@ -257,7 +259,12 @@
return wrapperSynthesizor
.getConversionCfProvider()
.generateInlinedAPIConversion(
- invoke, methodProcessingContext, localStackAllocator, eventConsumer, context);
+ invoke,
+ methodProcessingContext,
+ freshLocalProvider,
+ localStackAllocator,
+ eventConsumer,
+ context);
}
// If the option is set, we try to outline API conversions as much as possible to reduce the
@@ -265,6 +272,9 @@
// of soft verification failures. We cannot outline API conversions through super invokes, to
// instance initializers and to non public methods.
private boolean shouldOutlineAPIConversion(CfInvoke invoke, ProgramMethod context) {
+ if (appView.options().testing.forceInlineAPIConversions) {
+ return false;
+ }
if (invoke.isInvokeSuper(context.getHolderType())) {
return false;
}
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryConversionCfProvider.java b/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryConversionCfProvider.java
index 94bd06b..7907037 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryConversionCfProvider.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/desugaredlibrary/apiconversion/DesugaredLibraryConversionCfProvider.java
@@ -18,6 +18,7 @@
import com.android.tools.r8.cf.code.CfReturn;
import com.android.tools.r8.cf.code.CfStackInstruction;
import com.android.tools.r8.cf.code.CfStackInstruction.Opcode;
+import com.android.tools.r8.cf.code.CfStore;
import com.android.tools.r8.contexts.CompilationContext.MainThreadContext;
import com.android.tools.r8.contexts.CompilationContext.MethodProcessingContext;
import com.android.tools.r8.contexts.CompilationContext.UniqueContext;
@@ -36,6 +37,7 @@
import com.android.tools.r8.ir.code.MemberType;
import com.android.tools.r8.ir.code.ValueType;
import com.android.tools.r8.ir.desugar.CfInstructionDesugaringEventConsumer;
+import com.android.tools.r8.ir.desugar.FreshLocalProvider;
import com.android.tools.r8.ir.desugar.LocalStackAllocator;
import com.android.tools.r8.ir.desugar.desugaredlibrary.apiconversion.DesugaredLibraryWrapperSynthesizerEventConsumer.DesugaredLibraryAPICallbackSynthesizorEventConsumer;
import com.android.tools.r8.ir.desugar.desugaredlibrary.apiconversion.DesugaredLibraryWrapperSynthesizerEventConsumer.DesugaredLibraryAPIConverterEventConsumer;
@@ -265,6 +267,7 @@
public Collection<CfInstruction> generateInlinedAPIConversion(
CfInvoke invoke,
MethodProcessingContext methodProcessingContext,
+ FreshLocalProvider freshLocalProvider,
LocalStackAllocator localStackAllocator,
CfInstructionDesugaringEventConsumer eventConsumer,
ProgramMethod context) {
@@ -285,26 +288,34 @@
context,
methodProcessingContext::createUniqueContext);
- // If only the last 2 parameters require conversion, we do everything inlined.
- // If other parameters require conversion, we outline the parameter conversion but keep the API
- // call inlined.
- // The returned value is always converted inlined.
- boolean requireOutlinedParameterConversion = false;
- for (int i = 0; i < parameterConversions.length - 2; i++) {
- requireOutlinedParameterConversion |= parameterConversions[i] != null;
- }
-
+ int parameterSize = invokedMethod.getParameters().size();
ArrayList<CfInstruction> cfInstructions = new ArrayList<>();
- if (requireOutlinedParameterConversion) {
- addOutlineParameterConversionInstructions(
- parameterConversions,
- cfInstructions,
- methodProcessingContext,
- invokedMethod,
- localStackAllocator,
- eventConsumer);
- } else {
- addInlineParameterConversionInstructions(parameterConversions, cfInstructions);
+ if (parameterSize != 0) {
+ // If only the last 2 parameters require conversion, we do everything inlined.
+ // If other parameters require conversion, we outline the parameter conversion but keep the
+ // API
+ // call inlined. The returned value is always converted inlined.
+ boolean requireOutlinedParameterConversion = false;
+ for (int i = 0; i < parameterConversions.length - 2; i++) {
+ requireOutlinedParameterConversion |= parameterConversions[i] != null;
+ }
+ // We cannot use the swap instruction if the last parameter is wide.
+ requireOutlinedParameterConversion |=
+ invokedMethod.getParameters().get(parameterSize - 1).isWideType();
+
+ if (requireOutlinedParameterConversion) {
+ addOutlineParameterConversionInstructions(
+ parameterConversions,
+ cfInstructions,
+ methodProcessingContext,
+ invokedMethod,
+ freshLocalProvider,
+ localStackAllocator,
+ eventConsumer);
+ } else {
+ addInlineParameterConversionInstructions(
+ parameterConversions, cfInstructions, invokedMethod);
+ }
}
DexMethod convertedMethod =
@@ -325,6 +336,7 @@
ArrayList<CfInstruction> cfInstructions,
MethodProcessingContext methodProcessingContext,
DexMethod invokedMethod,
+ FreshLocalProvider freshLocalProvider,
LocalStackAllocator localStackAllocator,
CfInstructionDesugaringEventConsumer eventConsumer) {
localStackAllocator.allocateLocalStack(4);
@@ -353,8 +365,10 @@
eventConsumer.acceptAPIConversion(parameterConversion);
cfInstructions.add(
new CfInvoke(Opcodes.INVOKESTATIC, parameterConversion.getReference(), false));
+ int arrayLocal = freshLocalProvider.getFreshLocal(ValueType.OBJECT.requiredRegisters());
+ cfInstructions.add(new CfStore(ValueType.OBJECT, arrayLocal));
for (int i = 0; i < parameterConversions.length; i++) {
- cfInstructions.add(new CfStackInstruction(Opcode.Dup));
+ cfInstructions.add(new CfLoad(ValueType.OBJECT, arrayLocal));
cfInstructions.add(new CfConstNumber(i, ValueType.INT));
DexType parameterType =
parameterConversions[i] != null
@@ -368,9 +382,7 @@
} else {
cfInstructions.add(new CfCheckCast(parameterType));
}
- cfInstructions.add(new CfStackInstruction(Opcode.Swap));
}
- cfInstructions.add(new CfStackInstruction(Opcode.Pop));
}
private CfCode computeParameterConversionCfCode(
@@ -399,15 +411,13 @@
stackIndex++;
}
cfInstructions.add(new CfReturn(ValueType.OBJECT));
- return new CfCode(
- holder,
- invokedMethod.getParameters().size() + 4,
- invokedMethod.getParameters().size(),
- cfInstructions);
+ return new CfCode(holder, stackIndex + 4, stackIndex, cfInstructions);
}
private void addInlineParameterConversionInstructions(
- DexMethod[] parameterConversions, ArrayList<CfInstruction> cfInstructions) {
+ DexMethod[] parameterConversions,
+ ArrayList<CfInstruction> cfInstructions,
+ DexMethod invokedMethod) {
if (parameterConversions.length > 0
&& parameterConversions[parameterConversions.length - 1] != null) {
cfInstructions.add(
@@ -416,6 +426,10 @@
}
if (parameterConversions.length > 1
&& parameterConversions[parameterConversions.length - 2] != null) {
+ assert !invokedMethod
+ .getParameters()
+ .get(invokedMethod.getParameters().size() - 1)
+ .isWideType();
cfInstructions.add(new CfStackInstruction(Opcode.Swap));
cfInstructions.add(
new CfInvoke(
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index ca9c1e7..bc9d193 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1917,6 +1917,7 @@
public boolean neverReuseCfLocalRegisters = false;
public boolean roundtripThroughLIR = false;
public boolean checkReceiverAlwaysNullInCallSiteOptimization = true;
+ public boolean forceInlineAPIConversions = false;
private boolean hasReadCheckDeterminism = false;
private DeterminismChecker determinismChecker = null;
public boolean usePcEncodingInCfForTesting = false;
diff --git a/src/test/java/com/android/tools/r8/desugar/desugaredlibrary/conversiontests/BasicLongDoubleConversionTest.java b/src/test/java/com/android/tools/r8/desugar/desugaredlibrary/conversiontests/BasicLongDoubleConversionTest.java
index 3091b22..4659eba 100644
--- a/src/test/java/com/android/tools/r8/desugar/desugaredlibrary/conversiontests/BasicLongDoubleConversionTest.java
+++ b/src/test/java/com/android/tools/r8/desugar/desugaredlibrary/conversiontests/BasicLongDoubleConversionTest.java
@@ -4,7 +4,7 @@
package com.android.tools.r8.desugar.desugaredlibrary.conversiontests;
-import static com.android.tools.r8.desugar.desugaredlibrary.test.CompilationSpecification.DEFAULT_SPECIFICATIONS;
+import static com.android.tools.r8.desugar.desugaredlibrary.test.CompilationSpecification.SPECIFICATIONS_WITH_CF2CF;
import static com.android.tools.r8.desugar.desugaredlibrary.test.LibraryDesugaringSpecification.getJdk8Jdk11;
import com.android.tools.r8.TestParameters;
@@ -13,6 +13,7 @@
import com.android.tools.r8.desugar.desugaredlibrary.test.CustomLibrarySpecification;
import com.android.tools.r8.desugar.desugaredlibrary.test.LibraryDesugaringSpecification;
import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.StringUtils;
import java.time.MonthDay;
import java.util.List;
@@ -30,25 +31,29 @@
private final TestParameters parameters;
private final LibraryDesugaringSpecification libraryDesugaringSpecification;
private final CompilationSpecification compilationSpecification;
+ private final boolean forceInlineAPIConversions;
private static final AndroidApiLevel MIN_SUPPORTED = AndroidApiLevel.O;
- private static final String EXPECTED_RESULT = StringUtils.lines("--01-16");
+ private static final String EXPECTED_RESULT = StringUtils.lines("--01-16", "--01-02");
@Parameters(name = "{0}, spec: {1}, {2}")
public static List<Object[]> data() {
return buildParameters(
getConversionParametersUpToExcluding(MIN_SUPPORTED),
getJdk8Jdk11(),
- DEFAULT_SPECIFICATIONS);
+ SPECIFICATIONS_WITH_CF2CF,
+ BooleanUtils.values());
}
public BasicLongDoubleConversionTest(
TestParameters parameters,
LibraryDesugaringSpecification libraryDesugaringSpecification,
- CompilationSpecification compilationSpecification) {
+ CompilationSpecification compilationSpecification,
+ boolean forceInlineAPIConversions) {
this.parameters = parameters;
this.libraryDesugaringSpecification = libraryDesugaringSpecification;
this.compilationSpecification = compilationSpecification;
+ this.forceInlineAPIConversions = forceInlineAPIConversions;
}
@Test
@@ -59,7 +64,10 @@
new CustomLibrarySpecification(CustomLibClass.class, MIN_SUPPORTED))
.addKeepMainRule(Executor.class)
.addOptionsModification(options -> options.testing.trackDesugaredAPIConversions = true)
+ .addOptionsModification(
+ options -> options.testing.forceInlineAPIConversions = forceInlineAPIConversions)
.allowDiagnosticWarningMessages()
+ .compile()
.run(parameters.getRuntime(), Executor.class)
.assertSuccessWithOutput(EXPECTED_RESULT);
}
@@ -69,6 +77,7 @@
public static void main(String[] args) {
System.out.println(
CustomLibClass.mix(3L, 4L, MonthDay.of(1, 2), 5.0, 6.0, MonthDay.of(10, 20)));
+ System.out.println(CustomLibClass.convertThenWide(3L, 4L, MonthDay.of(1, 2), 5.0));
}
}
@@ -81,5 +90,11 @@
long l1, long l2, MonthDay monthDay1, double d1, double d2, MonthDay monthDay2) {
return monthDay1.withDayOfMonth((int) (monthDay2.getDayOfMonth() + l1 - d1 + l2 - d2));
}
+
+ // This tests the case where only the last but one parameter requires a conversion and the last
+ // parameter is wide.
+ public static MonthDay convertThenWide(long l1, long l2, MonthDay monthDay1, double d1) {
+ return monthDay1.withDayOfMonth((int) (l1 - d1 + l2));
+ }
}
}
