[PATCH] D45537: [CodeGenPrepare] Move Extension Instructions Through Logical And Shift Instructions

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3369
+       Inst->getOpcode() == Instruction::Or) &&
+      isa<ConstantInt>(Inst->getOperand(1)))
+    return true;
----------------
qcolombet wrote:
> Carrot wrote:
> > qcolombet wrote:
> > > Do we need to restrict this to constants?
> > It is same as DAGCombiner::visitZERO_EXTEND. Extension of constant doesn't cause extra cost at run time, it makes sure moving extension across this instruction doesn't bring another extension on the other operand. Only in rare case the extension on the other operand can also be combined with another load.
> Agreed. However, the check of the profitability of creating more than one extension is done later. Here we focus on answering, is it possible to extend?
> So constant or not, the answer should be yes (true).
> 
> My point is, unless you see a problem with the existing cost model used later, we shouldn't prevent the promotion to get evaluated.
Sounds reasonable.
I tried removing the constant check, there is only one failure test/CodeGen/X86/zext-logicop-shift-load.ll. 

The source code is:

; Don't do the folding if the other operand isn't a constant.
define i64 @test7(i8* %data, i8 %logop) {
; CHECK-LABEL: test7:
; CHECK:       movb
; CHECK-NEXT:  shrb
; CHECK-NEXT:  orb 
; CHECK-NEXT:  movzbl
; CHECK-NEXT:  retq
entry:
  %bf.load = load i8, i8* %data, align 4
  %bf.clear = lshr i8 %bf.load, 2
  %0 = or i8 %bf.clear, %logop
  %1 = zext i8 %0 to i64 
  ret i64 %1
}

Original result is:
        movb    (%rdi), %al 
        shrb    $2, %al 
        orb     %sil, %al 
        movzbl  %al, %eax
        retq

Now the result is:
        movzbl  (%rdi), %ecx
        shrq    $2, %rcx
        movzbl  %sil, %eax
        orq     %rcx, %rax
        retq

The new result isn't better than old result, neither worse. Do you think we should do transformation on this test case?


https://reviews.llvm.org/D45537