[InstCombine/InstSimplify] Move ashr optimization from Instcombine to Instsimplify

[suyog]

Summary

This patch moves the following optimization from visitAShr to SimplifyAShrInst :

// Arithmetic shifting an all-sign-bit value is a no-op.
unsigned NumSignBits = ComputeNumSignBits(Op0);
if (NumSignBits == Op0->getType()->getScalarSizeInBits())
  return Op0;

No regression observed. No functionality change.
This patch is part of refactoring code which i have planned along with Rahul Jain.

Primarily, we are targeting code which returns simply value instead of new instruction.

Any suggestions/help/pointers on the same will be very much appreciated.

Please help review this patch.

Thanks,
Suyog

[suyog]

Gentle Ping !!

[suyog]

Hi Nick,

Added supporting testcase.

Please if you could commit it for me if it looks
good to you.

Thanks,
Suyog

[suyog]

Hi Nick,

I modified the test case to use FileCheck.

I also believe the test case is in accordance to what we taught instsimplify as mentioned by Rahul above.
Also, this test case was the original one for instcombine, the code for which we moved to instsimplify.

Can you point out what's specific wrong in the test case? Your inputs are most welcomed :)

[suyog]

Small correction.

The test case is for 'instsimplify' rather than 'instcombine'.

Corrected the patch.

[suyog]

Gentle Ping !!

[suyog]

Gentle Ping !!

Nick, can you please help in reviewing it? Just a small patch :)

[nicholas]

Merge the test into something else (binops.ll perhaps)?

[nlewycky]

And LGTM. (I forgot that selecting "accept this revision" in the web UI
does not add LGTM to the email message.)

[suyog]

Closed by commit rL213231 (authored by @suyog).

llvm/trunk/lib/Analysis/InstructionSimplify.cpp

if (match(Op0, m_Undef()))
     return Constant::getAllOnesValue(Op0->getType());
 
   // (X << A) >> A -> X
   Value *X;
   if (match(Op0, m_Shl(m_Value(X), m_Specific(Op1))) &&
       cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap())
     return X;
 
+  // Arithmetic shifting an all-sign-bit value is a no-op.
+  unsigned NumSignBits = ComputeNumSignBits(Op0);
+  if (NumSignBits == Op0->getType()->getScalarSizeInBits())
+    return Op0;
+
   return nullptr;
 }
 
 Value *llvm::SimplifyAShrInst(Value *Op0, Value *Op1, bool isExact,
                               const DataLayout *DL,
                               const TargetLibraryInfo *TLI,
                               const DominatorTree *DT) {
   return ::SimplifyAShrInst(Op0, Op1, isExact, Query (DL, TLI, DT),

llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp

if (ConstantInt *Op1C = dyn_cast<ConstantInt>(Op1)) {
     }
   }
 
   // See if we can turn a signed shr into an unsigned shr.
   if (MaskedValueIsZero(Op0,
                         APInt::getSignBit(I.getType()->getScalarSizeInBits())))
     return BinaryOperator::CreateLShr(Op0, Op1);
 
-  // Arithmetic shifting an all-sign-bit value is a no-op.
-  unsigned NumSignBits = ComputeNumSignBits(Op0);
-  if (NumSignBits == Op0->getType()->getScalarSizeInBits())
-    return ReplaceInstUsesWith(I, Op0);
-
   return nullptr;
 }

llvm/trunk/test/Transforms/InstCombine/ashr-nop.ll

-; RUN: opt < %s -instcombine -S | not grep ashr
-
-define i32 @foo(i32 %x) {
-  %o = and i32 %x, 1
-  %n = add i32 %o, -1
-  %t = ashr i32 %n, 17
-  ret i32 %t
-}

llvm/trunk/test/Transforms/InstSimplify/ashr-nop.ll

+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+; CHECK-LABEL: @foo
+; CHECK-NOT: ashr
+define i32 @foo(i32 %x) {
+ %o = and i32 %x, 1
+ %n = add i32 %o, -1
+ %t = ashr i32 %n, 17
+ ret i32 %t
+}