[ConstantFolding] check applicability of AllOnes constant creation first

[vtjnash]

Summary

The getAllOnesValue can only handle things that are bitcast from a
ConstantInt, while here we bitcast through a pointer, so we may see more
complex objects (like Array or Struct).

[nathanchance]

I will throw this through my build tests for the Linux kernel and make sure that it passes (will probably take my machine four hours or so). I do not feel qualified enough to approve this once I get those results though.

Additionally, this should go into release/11.x since the branch just happened today.

[nickdesaulniers]

llvm/lib/Analysis/ConstantFolding.cpp

346–347

The mix of isa and method calls is inconsistent. How about we consistently use methods, rather than isa?

Type::isIntegerTy, Type::isVectorTy come to mind.

[nathanchance]

With this diff applied on top of f3731d34faa7432462c877714af235e9787c9b30, this looks good to me.

arm32 multi_v5_defconfig successful in 0:01:17

arm32 multi_v5_defconfig qemu boot successful

arm32 aspeed_g5_defconfig successful in 0:01:10

arm32 aspeed_g5_defconfig qemu boot successful

arm32 multi_v7_defconfig successful in 0:02:29

arm32 multi_v7_defconfig qemu boot successful

arm32 allmodconfig (plus CONFIG_CPU_BIG_ENDIAN=n) successful in 0:10:55

arm32 allnoconfig successful in 0:00:16

arm32 allyesconfig (plus CONFIG_CPU_BIG_ENDIAN=n) successful in 0:10:36

arm32 debian config successful in 0:07:25

armv7hl fedora config successful in 0:07:33

armv7hl opensuse config successful in 0:08:19

arm64 defconfig successful in 0:03:55

arm64 defconfig qemu boot successful

arm64 allmodconfig (plus CONFIG_CPU_BIG_ENDIAN=n) successful in 0:16:20

arm64 allnoconfig successful in 0:00:19

arm64 allyesconfig (plus CONFIG_CPU_BIG_ENDIAN=n) successful in 0:15:40

arm64 debian config successful in 0:10:10

arm64 fedora config successful in 0:10:13

arm64 opensuse config successful in 0:13:03

mips malta_kvm_guest_defconfig successful in 0:01:26

mips malta_kvm_guest_defconfig qemu boot successful

mips malta_kvm_guest_defconfig plus CONFIG_CPU_BIG_ENDIAN=y successful in 0:01:25

mips malta_kvm_guest_defconfig plus CONFIG_CPU_BIG_ENDIAN=y qemu boot successful

powerpc ppc44x_defconfig successful in 0:00:47

powerpc ppc44x_defconfig qemu boot successful

powerpc allnoconfig successful in 0:00:16

powerpc pseries_defconfig successful in 0:01:56

powerpc pseries_defconfig qemu boot successful

powerpc powernv_defconfig successful in 0:02:14

powerpc powernv_defconfig qemu boot successful

powerpc ppc64le_defconfig successful in 0:02:08

ppc64le debian config successful in 0:08:20

ppc64le fedora config successful in 0:08:23

ppc64le opensuse config successful in 0:09:11

riscv defconfig successful in 0:00:50

riscv defconfig qemu boot successful

s390x defconfig successful in 0:03:02

s390x allmodconfig successful in 0:10:03

s390x allyesconfig successful in 0:11:53

s390x debian config successful in 0:05:53

s390x fedora config successful in 0:03:52

s390x opensuse config successful in 0:04:24

x86_64 defconfig successful in 0:01:42

x86_64 qemu boot successful

x86_64 allmodconfig successful in 0:16:22

x86_64 allyesconfig successful in 0:15:27

x86_64 allyesconfig at -O3 successful in 0:16:19

x86_64 archlinux config successful in 0:10:34

x86_64 debian config successful in 0:10:47

x86_64 fedora config successful in 0:10:48

x86_64 opensuse config successful in 0:12:24

arm64 defconfig (plus CONFIG_{LTO,CFI}_CLANG and CONFIG_DYNAMIC_FTRACE_WITH_REGS) successful in 0:07:41

arm64 defconfig (plus CONFIG_{LTO,CFI}_CLANG and CONFIG_DYNAMIC_FTRACE_WITH_REGS) qemu boot successful

x86_64 defconfig (plus CONFIG_{LTO,CFI}_CLANG) successful in 0:02:20

x86_64 defconfig (plus CONFIG_{LTO,CFI}_CLANG) qemu boot successful

[vtjnash]

review

[vtjnash]

llvm/lib/Analysis/ConstantFolding.cpp

346–347

Works for me. I'd just followed getAllOnesValue, but agree this looks more consistent.

[nickdesaulniers]

Thanks for the quick turnaround on a fix!

[nathanchance]

Please ensure that this patch gets picked up into release/11.x as the patch that introduced this failure is in there.

[vtjnash]

Does that mean I should commit it there also, or how does that process work?

[nickdesaulniers]

Sedat filed a bug: https://bugs.llvm.org/show_bug.cgi?id=46785.

llvm/lib/Analysis/ConstantFolding.cpp

do {
     uint64_t SrcSize = DL.getTypeSizeInBits(SrcTy);
     if (SrcSize < DestSize)
       return nullptr;
 
     // Catch the obvious splat cases (since all-zeros can coerce non-integral
     // pointers legally).
     if (C->isNullValue() && !DestTy->isX86_MMXTy())
       return Constant::getNullValue(DestTy);
-    if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() &&
-        !DestTy->isPtrOrPtrVectorTy()) // Don't get ones for ptr types!
+    if (C->isAllOnesValue() &&
+        (DestTy->isIntegerTy() || DestTy->isFloatingPointTy() ||
+         DestTy->isVectorTy()) &&
+        !DestTy->isX86_MMXTy() && !DestTy->isPtrOrPtrVectorTy())
+      // Get ones when the input is trivial, but
+      // only for supported types inside getAllOnesValue.
       return Constant::getAllOnesValue(DestTy);
 
     // If the type sizes are the same and a cast is legal, just directly
     // cast the constant.
     // But be careful not to coerce non-integral pointers illegally.
     if (SrcSize == DestSize &&
         DL.isNonIntegralPointerType(SrcTy->getScalarType()) ==
             DL.isNonIntegralPointerType(DestTy->getScalarType())) {

llvm/test/Analysis/ConstantFolding/allones.ll

+; RUN: opt -early-cse -S -o - %s | FileCheck %s
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64-ni:2"
+target triple = "armv7-unknown-linux-gnueabi"
+
+%struct.anon = type { i32 }
+
+@onesstruct = private constant %struct.anon { i32 -1 }, align 4
+
+define i32 @allones_struct() {
+; CHECK-LABEL: @allones_struct()
+; CHECK-NEXT:    %1 = load [1 x i32], [1 x i32]* bitcast (%struct.anon* @onesstruct to [1 x i32]*), align 4
+; CHECK-NEXT:    %2 = extractvalue [1 x i32] %1, 0
+; CHECK-NEXT:    ret i32 %2
+  %1 = load [1 x i32], [1 x i32]* bitcast (%struct.anon* @onesstruct to [1 x i32]*), align 4
+  %2 = extractvalue [1 x i32] %1, 0
+  ret i32 %2
+}
+
+define i32 @allones_int() {
+; CHECK-LABEL: @allones_int()
+; CHECK-NEXT:    ret i32 -1
+  %1 = load i32, i32* bitcast (%struct.anon* @onesstruct to i32*), align 4
+  ret i32 %1
+}
+
+define i32* @allones_ptr() {
+; CHECK-LABEL: @allones_ptr()
+; CHECK-NEXT:    ret i32* inttoptr (i32 -1 to i32*)
+  %1 = load i32*, i32** bitcast (%struct.anon* @onesstruct to i32**), align 4
+  ret i32* %1
+}
+
+define i32 addrspace(1)* @allones_ptr1() {
+; CHECK-LABEL: @allones_ptr1()
+; CHECK-NEXT:    ret i32 addrspace(1)* inttoptr (i32 -1 to i32 addrspace(1)*)
+  %1 = load i32 addrspace(1)*, i32 addrspace(1)** bitcast (%struct.anon* @onesstruct to i32 addrspace(1)**), align 4
+  ret i32 addrspace(1)* %1
+}
+
+define i32 addrspace(2)* @allones_ptr2() {
+; CHECK-LABEL: @allones_ptr2()
+; CHECK-NEXT:    %1 = load i32 addrspace(2)*, i32 addrspace(2)** bitcast (%struct.anon* @onesstruct to i32 addrspace(2)**), align 4
+; CHECK-NEXT:    ret i32 addrspace(2)* %1
+  %1 = load i32 addrspace(2)*, i32 addrspace(2)** bitcast (%struct.anon* @onesstruct to i32 addrspace(2)**), align 4
+  ret i32 addrspace(2)* %1
+}