MachineConstantPool::getConstantPoolIndex - don't reuse mismatched constants contained undef/poison (Issue #63108)

[RKSimon]

Summary

This patch fixes an issue where we were reusing constant pool entries that contained undef elements, despite the additional uses of the 'equivalent constant' requiring some/all of the elements to be zero.

The CanShareConstantPoolEntry helper function uses ConstantFoldCastOperand to bitcasts the type mismatching constants to integer representations to allow comparison, but unfortunately this treats undef elements as zero (which they will be written out as in the final asm). This caused an issue where the original constant pool entry contained undef elements, which shared with a later constant that required the elements to be zero. This then caused a later analysis pass to incorrectly discard these undef elements.

Ideally we need a more thorough analysis / merging of the constant pool entries so the elements are forced to real zero elements, but for now we just prevent reuse of the constant pool entry entirely if the constants don't have matching undef/poison elements.

Fixes #63108

[RKSimon]

ping?

[goldstein.w.n]

re: "... but for now we just prevent reuse of the constant pool entry entirely."
->
".... but for now we just prevent reuse of the constant pool entry entirely *if there are any undef or poison elements*".

Otherwise, I don't think I really have the expertise to approve this, but FWIW LGTM.

[nikic]

LGTM

[efriedma]

Maybe worth intentionally noting that it's fine if B contains undef because if the check succeeds, we completely discard the constant B (so its contents don't matter).

It's a little weird that it's asymmetric like that, but I don't imagine it causes any immediate issue. (Merging constant pools isn't really that important at this level anyway because the linker also merges constant pools.)

llvm/lib/CodeGen/MachineFunction.cpp

for (const MachineConstantPoolEntry &C : Constants)
     }
   for (MachineConstantPoolValue *CPV : MachineCPVsSharingEntries) {
     if (Deleted.count(CPV) == 0)
       delete CPV;
   }
 }
 
 /// Test whether the given two constants can be allocated the same constant pool
-/// entry.
+/// entry referenced by \param A.
 static bool CanShareConstantPoolEntry(const Constant *A, const Constant *B,
                                       const DataLayout &DL) {
   // Handle the trivial case quickly.
   if (A == B) return true;
 
   // If they have the same type but weren't the same constant, quickly
   // reject them.
   if (A->getType() == B->getType()) return false;
 
   // We can't handle structs or arrays.
   if (isa<StructType>(A->getType()) || isa<ArrayType>(A->getType()) ||
       isa<StructType>(B->getType()) || isa<ArrayType>(B->getType()))
     return false;
 
   // For now, only support constants with the same size.
   uint64_t StoreSize = DL.getTypeStoreSize(A->getType());
   if (StoreSize != DL.getTypeStoreSize(B->getType()) || StoreSize > 128)
     return false;
 
+  bool ContainsUndefOrPoisonA = A->containsUndefOrPoisonElement();
+
   Type *IntTy = IntegerType::get(A->getContext(), StoreSize*8);
 
   // Try constant folding a bitcast of both instructions to an integer.  If we
   // get two identical ConstantInt's, then we are good to share them.  We use
   // the constant folding APIs to do this so that we get the benefit of
   // DataLayout.
   if (isa<PointerType>(A->getType()))
     A = ConstantFoldCastOperand(Instruction::PtrToInt,
                                 const_cast<Constant *>(A), IntTy, DL);
   else if (A->getType() != IntTy)
     A = ConstantFoldCastOperand(Instruction::BitCast, const_cast<Constant *>(A),
                                 IntTy, DL);
   if (isa<PointerType>(B->getType()))
     B = ConstantFoldCastOperand(Instruction::PtrToInt,
                                 const_cast<Constant *>(B), IntTy, DL);
   else if (B->getType() != IntTy)
     B = ConstantFoldCastOperand(Instruction::BitCast, const_cast<Constant *>(B),
                                 IntTy, DL);
 
-  return A == B;
+  if (A != B)
+    return false;
+
+  // Constants only safely match if A doesn't contain undef/poison.
+  // As we'll be reusing A, it doesn't matter if B contain undef/poison.
+  // TODO: Handle cases where A and B have the same undef/poison elements.
+  // TODO: Merge A and B with mismatching undef/poison elements.
+  return !ContainsUndefOrPoisonA;
 }
 
 /// Create a new entry in the constant pool or return an existing one.
 /// User must specify the log2 of the minimum required alignment for the object.
 unsigned MachineConstantPool::getConstantPoolIndex(const Constant *C,
                                                    Align Alignment) {
   if (Alignment > PoolAlignment) PoolAlignment = Alignment;
 

llvm/test/CodeGen/X86/pr63108.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=SSE
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx | FileCheck %s --check-prefixes=AVX1
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx2 | FileCheck %s --check-prefixes=AVX2
 ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=x86-64-v4 | FileCheck %s --check-prefixes=AVX512
 
-; FIXME: %vector.body.preheader <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u> constant should be <251,223,0,0,0,0,0,0,0,0,0,0,0,0,0,0>
-
 define i32 @PR63108() {
 ; SSE-LABEL: PR63108:
 ; SSE:       # %bb.0: # %entry
 ; SSE-NEXT:    xorl %eax, %eax
 ; SSE-NEXT:    testb %al, %al
 ; SSE-NEXT:    je .LBB0_2
 ; SSE-NEXT:  # %bb.1:
 ; SSE-NEXT:    movdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
 ; SSE-NEXT:    jmp .LBB0_5
 ; SSE-NEXT:  .LBB0_2: # %vector.body.preheader
 ; SSE-NEXT:    pxor %xmm0, %xmm0
-; SSE-NEXT:    movdqa {{.*#+}} xmm2 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
+; SSE-NEXT:    movdqa {{.*#+}} xmm2 = [57339,0,0,0]
 ; SSE-NEXT:    xorl %eax, %eax
 ; SSE-NEXT:    .p2align 4, 0x90
 ; SSE-NEXT:  .LBB0_3: # %vector.body
 ; SSE-NEXT:    # =>This Inner Loop Header: Depth=1
 ; SSE-NEXT:    movdqa %xmm2, %xmm1
 ; SSE-NEXT:    testb %al, %al
 ; SSE-NEXT:    pxor %xmm2, %xmm2
 ; SSE-NEXT:    jne .LBB0_3

 ; AVX1:       # %bb.0: # %entry
 ; AVX1-NEXT:    xorl %eax, %eax
 ; AVX1-NEXT:    testb %al, %al
 ; AVX1-NEXT:    je .LBB0_2
 ; AVX1-NEXT:  # %bb.1:
 ; AVX1-NEXT:    vmovdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
 ; AVX1-NEXT:    jmp .LBB0_5
 ; AVX1-NEXT:  .LBB0_2: # %vector.body.preheader
-; AVX1-NEXT:    vmovaps {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
+; AVX1-NEXT:    vmovaps {{.*#+}} xmm0 = [57339,0,0,0]
 ; AVX1-NEXT:    xorl %eax, %eax
 ; AVX1-NEXT:    .p2align 4, 0x90
 ; AVX1-NEXT:  .LBB0_3: # %vector.body
 ; AVX1-NEXT:    # =>This Inner Loop Header: Depth=1
 ; AVX1-NEXT:    vmovaps %ymm0, %ymm1
 ; AVX1-NEXT:    vxorps %xmm0, %xmm0, %xmm0
 ; AVX1-NEXT:    testb %al, %al
 ; AVX1-NEXT:    jne .LBB0_3

 ; AVX2:       # %bb.0: # %entry
 ; AVX2-NEXT:    xorl %eax, %eax
 ; AVX2-NEXT:    testb %al, %al
 ; AVX2-NEXT:    je .LBB0_2
 ; AVX2-NEXT:  # %bb.1:
 ; AVX2-NEXT:    vmovdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
 ; AVX2-NEXT:    jmp .LBB0_5
 ; AVX2-NEXT:  .LBB0_2: # %vector.body.preheader
-; AVX2-NEXT:    vmovdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
+; AVX2-NEXT:    vmovdqa {{.*#+}} xmm0 = [57339,0,0,0]
 ; AVX2-NEXT:    xorl %eax, %eax
 ; AVX2-NEXT:    .p2align 4, 0x90
 ; AVX2-NEXT:  .LBB0_3: # %vector.body
 ; AVX2-NEXT:    # =>This Inner Loop Header: Depth=1
 ; AVX2-NEXT:    vmovdqa %ymm0, %ymm1
 ; AVX2-NEXT:    vpxor %xmm0, %xmm0, %xmm0
 ; AVX2-NEXT:    testb %al, %al
 ; AVX2-NEXT:    jne .LBB0_3

 ; AVX512:       # %bb.0: # %entry
 ; AVX512-NEXT:    xorl %eax, %eax
 ; AVX512-NEXT:    testb %al, %al
 ; AVX512-NEXT:    je .LBB0_2
 ; AVX512-NEXT:  # %bb.1:
 ; AVX512-NEXT:    vmovdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
 ; AVX512-NEXT:    jmp .LBB0_5
 ; AVX512-NEXT:  .LBB0_2: # %vector.body.preheader
-; AVX512-NEXT:    vmovdqa {{.*#+}} xmm0 = <251,223,u,u,u,u,u,u,u,u,u,u,u,u,u,u>
+; AVX512-NEXT:    vmovdqa {{.*#+}} xmm0 = [57339,0,0,0]
 ; AVX512-NEXT:    xorl %eax, %eax
 ; AVX512-NEXT:    .p2align 4, 0x90
 ; AVX512-NEXT:  .LBB0_3: # %vector.body
 ; AVX512-NEXT:    # =>This Inner Loop Header: Depth=1
 ; AVX512-NEXT:    vmovdqa %ymm0, %ymm1
 ; AVX512-NEXT:    vpxor %xmm0, %xmm0, %xmm0
 ; AVX512-NEXT:    testb %al, %al
 ; AVX512-NEXT:    jne .LBB0_3