[PATCH] D11408: [ARM/AArch64] - Remove some undefined behaviour when lowering vector shifts

[Luke Cheeseman]

LukeCheeseman created this revision.
LukeCheeseman added a subscriber: llvm-commits.
Herald added subscribers: rengolin, aemerson.

When lowering vector shifts a check is performed to see if the value to shift by is an immediate, in this check the value is negated and stored in and int64_t. The value can be -2^63 yet the result cannot be stored in an int64_t and this gives some undefined behaviour. The regression test included exposes this behaviour, this test fails in the current release build with assertions when built using clang as the check for an immediate passes when it should not due to this undefined behaviour. The negation is only necessary when the values is within a certain range and so it should not need to negate -2^63, this patch introduces this and also a regression test.

http://reviews.llvm.org/D11408

Files:
  lib/Target/AArch64/AArch64ISelLowering.cpp
  lib/Target/ARM/ARMISelLowering.cpp
  test/CodeGen/ARM/neon_vshl_minint.ll

Index: test/CodeGen/ARM/neon_vshl_minint.ll
===================================================================
--- test/CodeGen/ARM/neon_vshl_minint.ll
+++ test/CodeGen/ARM/neon_vshl_minint.ll
@@ -0,0 +1,13 @@
+; RUN: llc < %s -mtriple=arm-none-eabi -mcpu=cortex-a57 2>&1 | FileCheck %s --check-prefix=ARM --check-prefix=ACORE
+; RUN: llc < %s -mtriple=thumb-none-eabi -mcpu=cortex-a57 2>&1 | FileCheck %s --check-prefix=ARM --check-prefix=MCORE
+
+define <1 x i64> @vshl_minint() #0 {
+  entry:
+    ; ARM-LABEL: vshl_minint
+    ; ARM: vldr
+    ; ARM: vshl.u64
+    %vshl.i = tail call <1 x i64> @llvm.arm.neon.vshiftu.v1i64(<1 x i64> undef, <1 x i64> <i64 -9223372036854775808>)
+    ret <1 x i64> %vshl.i
+}
+
+declare <1 x i64> @llvm.arm.neon.vshiftu.v1i64(<1 x i64>, <1 x i64>)
Index: lib/Target/ARM/ARMISelLowering.cpp
===================================================================
--- lib/Target/ARM/ARMISelLowering.cpp
+++ lib/Target/ARM/ARMISelLowering.cpp
@@ -9691,7 +9691,7 @@
 ///   0 <= Value <= ElementBits for a long left shift.
 static bool isVShiftLImm(SDValue Op, EVT VT, bool isLong, int64_t &Cnt) {
   assert(VT.isVector() && "vector shift count is not a vector type");
-  unsigned ElementBits = VT.getVectorElementType().getSizeInBits();
+  int64_t ElementBits = VT.getVectorElementType().getSizeInBits();
   if (! getVShiftImm(Op, ElementBits, Cnt))
     return false;
   return (Cnt >= 0 && (isLong ? Cnt-1 : Cnt) < ElementBits);
@@ -9706,12 +9706,16 @@
 static bool isVShiftRImm(SDValue Op, EVT VT, bool isNarrow, bool isIntrinsic,
                          int64_t &Cnt) {
   assert(VT.isVector() && "vector shift count is not a vector type");
-  unsigned ElementBits = VT.getVectorElementType().getSizeInBits();
+  int64_t ElementBits = VT.getVectorElementType().getSizeInBits();
   if (! getVShiftImm(Op, ElementBits, Cnt))
     return false;
-  if (isIntrinsic)
+  if (!isIntrinsic)
+    return (Cnt >= 1 && Cnt <= (isNarrow ? ElementBits/2 : ElementBits));
+  if (Cnt >= -(isNarrow ? ElementBits/2 : ElementBits) && Cnt <= -1) {
     Cnt = -Cnt;
-  return (Cnt >= 1 && Cnt <= (isNarrow ? ElementBits/2 : ElementBits));
+    return true;
+  }
+  return false;
 }
 
 /// PerformIntrinsicCombine - ARM-specific DAG combining for intrinsics.
Index: lib/Target/AArch64/AArch64ISelLowering.cpp
===================================================================
--- lib/Target/AArch64/AArch64ISelLowering.cpp
+++ lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -6440,7 +6440,7 @@
 ///   0 <= Value <= ElementBits for a long left shift.
 static bool isVShiftLImm(SDValue Op, EVT VT, bool isLong, int64_t &Cnt) {
   assert(VT.isVector() && "vector shift count is not a vector type");
-  unsigned ElementBits = VT.getVectorElementType().getSizeInBits();
+  int64_t ElementBits = VT.getVectorElementType().getSizeInBits();
   if (!getVShiftImm(Op, ElementBits, Cnt))
     return false;
   return (Cnt >= 0 && (isLong ? Cnt - 1 : Cnt) < ElementBits);
@@ -6455,12 +6455,16 @@
 static bool isVShiftRImm(SDValue Op, EVT VT, bool isNarrow, bool isIntrinsic,
                          int64_t &Cnt) {
   assert(VT.isVector() && "vector shift count is not a vector type");
-  unsigned ElementBits = VT.getVectorElementType().getSizeInBits();
+  int64_t ElementBits = VT.getVectorElementType().getSizeInBits();
   if (!getVShiftImm(Op, ElementBits, Cnt))
     return false;
-  if (isIntrinsic)
+  if (!isIntrinsic)
+    return (Cnt >= 1 && Cnt <= (isNarrow ? ElementBits/2 : ElementBits));
+  if (Cnt >= -(isNarrow ? ElementBits/2 : ElementBits) && Cnt <= -1) {
     Cnt = -Cnt;
-  return (Cnt >= 1 && Cnt <= (isNarrow ? ElementBits / 2 : ElementBits));
+    return true;
+  }
+  return false;
 }
 
 SDValue AArch64TargetLowering::LowerVectorSRA_SRL_SHL(SDValue Op,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D11408.30335.patch
Type: text/x-patch
Size: 3783 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150722/404fdc2f/attachment.bin>