[X86] Fix a bug with fetch_add(INT32_MIN)

[morisset]

Summary

Fix pr21099

The pseudocode of what we were doing (spread through two functions) was:

if (operand.doesNotFitIn32Bits())
  Opc.initializeWithFoo();
if (operand < 0)
  operand = -operand;
if (operand.doesFitIn8Bits())
  Opc.initializeWithBar();
else if (operand.doesFitIn32Bits())
  Opc.initializeWithBlah();
doStuff(Opc);

So for operand == INT32_MIN, Opc was never initialized because the operand changes
from fitting in 32 bits to not fitting, causing the various bugs/error messages
noted by pr21099.

This patch adds an extra test at the beginning for this case, and an
llvm_unreachable to have better error message if the operand ends up
not fitting in 32-bits at the end.

[morisset]

Based on an offline comment by jf, improve the patch so that the immediate is
merged into the addq.

[jfb]

lib/Target/X86/X86ISelDAGToDAG.cpp

1717 ↗

(On Diff #14533)

The key thing to understand is that negating INT32_MIN makes it not fit in 32-bits. I'm not sure the comment makes that clear.

1718 ↗

(On Diff #14533)

It looks like the prevalent thing is to use INT32_MIN from <stdint.h> instead of std::numeric_limits. I know I pointed you at std::numeric_limits, but I think we should follow what other parts of LLVM use.

[morisset]

Apply jfb suggestions.

[jfb]

lgtm

[morisset]

Closed by commit rL219257 (authored by @morisset).

llvm/trunk/lib/Target/X86/X86ISelDAGToDAG.cpp

 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
+#include <stdint.h>
 using namespace llvm;
 
 #define DEBUG_TYPE "x86-isel"
 
 STATISTIC(NumLoadMoved, "Number of loads moved below TokenFactor");
 
 //===----------------------------------------------------------------------===//
 //                      Pattern Matcher Implementation
static SDValue getAtomicLoadArithTargetConstant(SelectionDAG *CurDAG,
                                                 SDLoc dl,
                                                 enum AtomicOpc &Op, MVT NVT,
                                                 SDValue Val) {
   if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Val)) {
     int64_t CNVal = CN->getSExtValue();
     // Quit if not 32-bit imm.
     if ((int32_t)CNVal != CNVal)
       return Val;
+    // Quit if INT32_MIN: it would be negated as it is negative and overflow,
+    // producing an immediate that does not fit in the 32 bits available for
+    // an immediate operand to sub. However, it still fits in 32 bits for the
+    // add (since it is not negated) so we can return target-constant.
+    if (CNVal == INT32_MIN)
+      return CurDAG->getTargetConstant(CNVal, NVT);
     // For atomic-load-add, we could do some optimizations.
     if (Op == ADD) {
       // Translate to INC/DEC if ADD by 1 or -1.
       if ((CNVal == 1) || (CNVal == -1)) {
         Op = (CNVal == 1) ? INC : DEC;
         // No more constant operand after being translated into INC/DEC.
         return SDValue();
       }
case MVT::i32:
         Opc = AtomicOpcTbl[Op][I32];
       break;
     case MVT::i64:
       if (isCN) {
         if (immSext8(Val.getNode()))
           Opc = AtomicOpcTbl[Op][SextConstantI64];
         else if (i64immSExt32(Val.getNode()))
           Opc = AtomicOpcTbl[Op][ConstantI64];
+        else
+          llvm_unreachable("True 64 bits constant in SelectAtomicLoadArith");
       } else
         Opc = AtomicOpcTbl[Op][I64];
       break;
   }
 
   assert(Opc != 0 && "Invalid arith lock transform!");
 
   // Building the new node.

llvm/trunk/test/CodeGen/X86/pr21099.ll

+; RUN: llc < %s -O2 -march=x86-64 -verify-machineinstrs | FileCheck %s
+
+define void @pr21099(i64* %p) {
+; CHECK-LABEL: pr21099
+; CHECK: lock
+; CHECK-NEXT: addq $-2147483648
+; This number is INT32_MIN: 0x80000000UL
+  %1 = atomicrmw add i64* %p, i64 -2147483648 seq_cst
+  ret void
+}