[InstCombine] Don't transform (select c, ext(TI), ext(FI)) if ext is free

[Carrot]

Summary

Function InstCombinerImpl::foldSelectOpOp can do following transformation

(select c, ext(TI), ext(FI))  ==> (ext(select c, TI, FI))

If TI and FI are load, then ext is free on x86, in this situation we should not move ext out of select.

[lebedev.ri]

InstCombine is a canonicalization pass, and by design it must not make use of TTI.
This should be fixed by a reverse transformation in backend.

[Carrot]

Thanks for the info.

I will discard this patch and reverse the pattern at function CodeGenPrepare::optimizeExt.

[lebedev.ri]

In D94851#2507867, @Carrot wrote:

Thanks for the info.

I will discard this patch and reverse the pattern at function CodeGenPrepare::optimizeExt.

Not really sure why that would go into CodeGenPrepare, which is a temporary hack,
DAGCombine is the usual counterpart.

[Carrot]

In D94851#2507869, @lebedev.ri wrote:

In D94851#2507867, @Carrot wrote:

Thanks for the info.

I will discard this patch and reverse the pattern at function CodeGenPrepare::optimizeExt.

Not really sure why that would go into CodeGenPrepare, which is a temporary hack,
DAGCombine is the usual counterpart.

Because I found CodeGenPrepare::optimizeExt does some ext(load( )) optimization.
Will look into DAGCombine.
Thanks.

llvm/include/llvm/Transforms/InstCombine/InstCombiner.h

 class TargetLibraryInfo;
 class TargetTransformInfo;
 
 /// The core instruction combiner logic.
 ///
 /// This class provides both the logic to recursively visit instructions and
 /// combine them.
 class LLVM_LIBRARY_VISIBILITY InstCombiner {
+protected:
   /// Only used to call target specific inst combining.
   TargetTransformInfo &TTI;
 
 public:
   /// Maximum size of array considered when transforming.
   uint64_t MaxArraySizeForCombine = 0;
 
   /// An IRBuilder that automatically inserts new instructions into the

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

 #include "InstCombineInternal.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/CmpInstAnalysis.h"
 #include "llvm/Analysis/InstructionSimplify.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
if (auto *CondVTy = dyn_cast<VectorType>(CondTy)) {
         return nullptr;
     } else if (!TI->hasOneUse() || !FI->hasOneUse()) {
       // TODO: The one-use restrictions for a scalar select could be eased if
       // the fold of a select in visitLoadInst() was enhanced to match a pattern
       // that includes a cast.
       return nullptr;
     }
 
+    // If both cast instructions are free, don't merge them.
+    auto Opcode = TI->getOpcode();
+    if (TTI.getCastInstrCost(Opcode, TI->getType(), FIOpndTy,
+                             TTI.getCastContextHint(TI)) == 0 &&
+        TTI.getCastInstrCost(Opcode, FI->getType(), FIOpndTy,
+                             TTI.getCastContextHint(FI)) == 0)
+      return nullptr;
+
     // Fold this by inserting a select from the input values.
     Value *NewSI =
         Builder.CreateSelect(Cond, TI->getOperand(0), FI->getOperand(0),
                              SI.getName() + ".v", &SI);
     return CastInst::Create(Instruction::CastOps(TI->getOpcode()), NewSI,
                             TI->getType());
   }
 

llvm/test/Transforms/InstCombine/select-ext.ll

+; RUN: opt -instcombine -mtriple=x86_64-unknown-unknown -S < %s | FileCheck %s
+
+; If zext has 0 cost, it should not be moved after select.
+
+; CHECK-LABEL: @foo
+; CHECK:        [[LD1:%.*]] = load volatile i8, i8* [[PTR1:%.*]]
+; CHECK:        [[CONV1:%.*]] = zext i8 [[LD1]] to i64
+; CHECK:        [[LD2:%.*]] = load volatile i8, i8* [[PTR2:%.*]]
+; CHECK:        [[CONV2:%.*]] = zext i8 [[LD2]] to i64
+; CHECK:        [[RESULT:%.*]] = select i1 [[COND:%.*]], i64 [[CONV2]], i64 [[CONV1]]
+; CHECK:        ret i64 [[RESULT]]
+
+define i64 @foo(i8* %p, i1 zeroext %c) {
+  %ld1 = load volatile i8, i8* %p
+  %conv = zext i8 %ld1 to i64
+  %arrayidx1 = getelementptr inbounds i8, i8* %p, i64 1
+  %ld2 = load volatile i8, i8* %arrayidx1
+  %conv2 = zext i8 %ld2 to i64
+  %cond = select i1 %c, i64 %conv2, i64 %conv
+  ret i64 %cond
+}
+