[ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation

[iid_iunknown]

Summary

A temporary workaround for PR32780 - rematerialized instructions accessing the same promoted global through different constant pool entries.

The patch turns off the globals promotion optimization leaving all its code in place, so that it can be easily turned on once PR32780 is fixed.

Since this is a miscompilation issue causing generation of misbehaving code, and the problem is very subtle, the patch might be valuable enough to get into 4.0.1.

[efriedma]

I would suggest switching the option (arm-promote-constant) to default to false rather than commenting out the code, so we don't have to XFAIL the unit-tests.

At a higher level, this seems okay.

[iid_iunknown]

In D33446#762242, @efriedma wrote:

I would suggest switching the option (arm-promote-constant) to default to false rather than commenting out the code, so we don't have to XFAIL the unit-tests.

Thanks for your comment, Eli.

Yes, initially I thought about option adjusting too as it's easier to do, but refused this idea because one might hit the unexpected miscompilation if setting the option to true explicitly in the command line. One will get no warning about possible issues with this option.

Do you confirm that such behavior is acceptable?

[efriedma]

Do you confirm that such behavior is acceptable?

Yes, this is fine; we switch off optimizations which aren't ready to use with options all the time.

[jmolloy]

+1 from me.

[iid_iunknown]

Addressing the review comments.

[iid_iunknown]

Updated the patch.

Extra corrections:

• added a FIXME comment to revert the option to true when the problem is resolved;
• added explicit -arm-promote-constant to the tests as they expect promotion to happen.

[efriedma]

LGTM.

[iid_iunknown]

Eli, James, thanks for the review!

lib/Target/ARM/ARMISelLowering.cpp

 ARMInterworking("arm-interworking", cl::Hidden,
   cl::desc("Enable / disable ARM interworking (for debugging only)"),
   cl::init(true));
 
 static cl::opt<bool> EnableConstpoolPromotion(
     "arm-promote-constant", cl::Hidden,
     cl::desc("Enable / disable promotion of unnamed_addr constants into "
              "constant pools"),
-    cl::init(true));
+    cl::init(false)); // FIXME: set to true by default once PR32780 is fixed
 static cl::opt<unsigned> ConstpoolPromotionMaxSize(
     "arm-promote-constant-max-size", cl::Hidden,
     cl::desc("Maximum size of constant to promote into a constant pool"),
     cl::init(64));
 static cl::opt<unsigned> ConstpoolPromotionMaxTotal(
     "arm-promote-constant-max-total", cl::Hidden,
     cl::desc("Maximum size of ALL constants to promote into a constant pool"),
     cl::init(128));

test/CodeGen/ARM/constantpool-promote-dbg.ll

-; RUN: llc -relocation-model=static < %s | FileCheck %s
+; RUN: llc -relocation-model=static -arm-promote-constant < %s | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "thumbv7m--linux-gnu"
 
 @.str = private unnamed_addr constant [4 x i8] c"abc\00", align 1
 
 ; CHECK-LABEL: fn1
 ; CHECK: .str:

test/CodeGen/ARM/constantpool-promote-ldrh.ll

-; RUN: llc < %s -O0 -fast-isel=false | FileCheck %s
-; RUN: llc < %s -O0 -fast-isel=false -filetype=obj
+; RUN: llc < %s -O0 -fast-isel=false -arm-promote-constant | FileCheck %s
+; RUN: llc < %s -O0 -fast-isel=false -filetype=obj -arm-promote-constant
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "thumbv6m-arm-linux-gnueabi"
 
 @fn1.a = private unnamed_addr constant [4 x i16] [i16 6, i16 0, i16 0, i16 0], align 2
 
 ; We must not try and emit this bad instruction: "ldrh r1, .LCPI0_0"
 ; CHECK-LABEL: fn1:
 ; CHECK: adr [[base:r[0-9]+]], .LCPI0_0

test/CodeGen/ARM/constantpool-promote.ll

-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=static < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=pic < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=ropi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
-; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=rwpi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
-; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=static < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
-; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=pic < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
-; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=ropi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
-; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=rwpi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
-; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=static < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
-; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=pic < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
-; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=ropi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
-; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=rwpi < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
+; RUN: llc -mtriple armv7--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7ARM
+; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
+; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
+; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
+; RUN: llc -mtriple thumbv7--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V7,CHECK-V7THUMB
+; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=static -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
+; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=pic -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
+; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=ropi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
+; RUN: llc -mtriple thumbv6m--linux-gnueabihf -relocation-model=rwpi -arm-promote-constant < %s | FileCheck %s --check-prefixes=CHECK,CHECK-V6M
 
 @.str = private unnamed_addr constant [2 x i8] c"s\00", align 1
 @.str1 = private unnamed_addr constant [69 x i8] c"this string is far too long to fit in a literal pool by far and away\00", align 1
 @.str2 = private unnamed_addr constant [27 x i8] c"this string is just right!\00", align 1
 @.str3 = private unnamed_addr constant [26 x i8] c"this string is used twice\00", align 1
 @.str4 = private unnamed_addr constant [29 x i8] c"same string in two functions\00", align 1
 @.arr1 = private unnamed_addr constant [2 x i16] [i16 3, i16 4], align 2
 @.arr2 = private unnamed_addr constant [2 x i16] [i16 7, i16 8], align 2