Allow PeepholeOptimizer to fold a few more cases

[mkuper]

Summary

The current condition looks overly conservative.
I don't see why an IMPLICIT_DEF, for instance, should clear the folding candidate set, and the same for KILL.
Two things I'm not sure of:

1. Should isPosition() be a barrier?
2. Is the explicit check for isInlineAsm() really necessary? (InlineAsm should correctly report mayStore()/hasUnmodeledSideEffects(), right?)

Andrew, I've added you to the review because of the rdar mentioned in the test-case. The folding there currently didn't happen due to an intervening IMPLICIT_DEF, and not because of any scheduling issues. Am I missing something?

[mkuper]

Ping

[qcolombet]

Hi Michael,

I believe the current situation is indeed an historical mistake :).

Regarding the load folding, no, I do not believe isPosition should be a barrier, probably the same for inline asm. The thing about inline asm is that if the user forgot the volatile keyword whereas it was doing unmodeled side effect, we may not model the right thing. That being said, I guess this is a mistake from the user side and we do not need to be conservative in such case.

I’d say update your patch to reflect that.

Side note, could you be more specific in the test cases regular expressions?
I.e., if you expect a memory location for a given parameter now, instead of a register, try to reflect the in the expression so that if we regress that we will see it.

Thanks,
-Quentin

[atrick]

I confirmed that this fixes the test case mentioned in the radar. Many issues were exposed by changing the default scheduler, but were not actually scheduling issues. Thanks for fixing it.

[mkuper]

Thanks, Quentin!
Removed the inlineAsm condition.

As to the tests - they already check for memory references. ({{.+}}) won't match a register.
In fact, this is the way the test originally was, when it checked for folding, before the scheduler breakage.

[qcolombet]

As to the tests - they already check for memory references. ({{.+}}) won't match a register.

Oh, that’s right, I missed the parentheses :).

LGTM then.

Thanks,
-Quentin

lib/CodeGen/PeepholeOptimizer.cpp

for (MachineBasicBlock::iterator
       LocalMIs.insert(MI);
 
       // Skip debug values. They should not affect this peephole optimization.
       if (MI->isDebugValue())
           continue;
 
       // If there exists an instruction which belongs to the following
       // categories, we will discard the load candidates.
+      if (MI->mayStore() || MI->isCall() || MI->hasUnmodeledSideEffects() ||
+          MI->isInlineAsm())
+        FoldAsLoadDefCandidates.clear();
+
       if (MI->isPosition() || MI->isPHI() || MI->isImplicitDef() ||
           MI->isKill() || MI->isInlineAsm() ||
-          MI->hasUnmodeledSideEffects()) {
-        FoldAsLoadDefCandidates.clear();
+          MI->hasUnmodeledSideEffects())
         continue;
-      }
-      if (MI->mayStore() || MI->isCall())
-        FoldAsLoadDefCandidates.clear();
 
       if ((isUncoalescableCopy(*MI) &&
            optimizeUncoalescableCopy(MI, LocalMIs)) ||
           (MI->isCompare() && optimizeCmpInstr(MI, MBB)) ||
           (MI->isSelect() && optimizeSelect(MI, LocalMIs))) {
         // MI is deleted.
         LocalMIs.erase(MI);
         Changed = true;

test/CodeGen/X86/avx-cvt.ll

; CHECK-NEXT:    retq
   %tmp1 = load i64, i64* %e, align 8
   %conv = sitofp i64 %tmp1 to float
   ret float %conv
 }
 
 define void @fpext() nounwind uwtable {
 ; CHECK-LABEL: fpext:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    vmovss {{.*#+}} xmm0 = mem[0],zero,zero,zero
-; CHECK-NEXT:    vcvtss2sd %xmm0, %xmm0, %xmm0
+; CHECK-NEXT:    vcvtss2sd -{{[0-9]+}}(%rsp), %xmm0, %xmm0
 ; CHECK-NEXT:    vmovsd %xmm0, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    retq
   %f = alloca float, align 4
   %d = alloca double, align 8
   %tmp = load float, float* %f, align 4
   %conv = fpext float %tmp to double
   store double %conv, double* %d, align 8
   ret void

test/CodeGen/X86/shift-bmi2.ll

; BMI264: ret
   ret i32 %shl
 }
 
 define i32 @shl32p(i32* %p, i32 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i32, i32* %p
   %shl = shl i32 %x, %shamt
 ; BMI2: shl32p
-; Source order scheduling prevents folding, rdar:14208996.
-; BMI2: shlxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI2: shlxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI2: ret
 ; BMI264: shl32p
-; BMI264: shlxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: shlxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i32 %shl
 }
 
 define i32 @shl32pi(i32* %p) nounwind uwtable readnone {
 entry:
   %x = load i32, i32* %p
   %shl = shl i32 %x, 5
; BMI264: ret
   ret i64 %shl
 }
 
 define i64 @shl64p(i64* %p, i64 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i64, i64* %p
   %shl = shl i64 %x, %shamt
 ; BMI264: shl64p
-; BMI264: shlxq %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: shlxq %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i64 %shl
 }
 
 define i64 @shl64pi(i64* %p) nounwind uwtable readnone {
 entry:
   %x = load i64, i64* %p
   %shl = shl i64 %x, 7
; BMI264: ret
   ret i32 %shl
 }
 
 define i32 @lshr32p(i32* %p, i32 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i32, i32* %p
   %shl = lshr i32 %x, %shamt
 ; BMI2: lshr32p
-; Source order scheduling prevents folding, rdar:14208996.
-; BMI2: shrxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI2: shrxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI2: ret
 ; BMI264: lshr32p
-; BMI264: shrxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: shrxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i32 %shl
 }
 
 define i64 @lshr64(i64 %x, i64 %shamt) nounwind uwtable readnone {
 entry:
   %shl = lshr i64 %x, %shamt
 ; BMI264: lshr64
 ; BMI264: shrxq
 ; BMI264: ret
   ret i64 %shl
 }
 
 define i64 @lshr64p(i64* %p, i64 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i64, i64* %p
   %shl = lshr i64 %x, %shamt
 ; BMI264: lshr64p
-; BMI264: shrxq %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: shrxq %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i64 %shl
 }
 
 define i32 @ashr32(i32 %x, i32 %shamt) nounwind uwtable readnone {
 entry:
   %shl = ashr i32 %x, %shamt
 ; BMI2: ashr32
 ; BMI2: sarxl
 ; BMI2: ret
 ; BMI264: ashr32
 ; BMI264: sarxl
 ; BMI264: ret
   ret i32 %shl
 }
 
 define i32 @ashr32p(i32* %p, i32 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i32, i32* %p
   %shl = ashr i32 %x, %shamt
 ; BMI2: ashr32p
 ; Source order scheduling prevents folding, rdar:14208996.
-; BMI2: sarxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI2: sarxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI2: ret
 ; BMI264: ashr32p
-; BMI264: sarxl %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: sarxl %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i32 %shl
 }
 
 define i64 @ashr64(i64 %x, i64 %shamt) nounwind uwtable readnone {
 entry:
   %shl = ashr i64 %x, %shamt
 ; BMI264: ashr64
 ; BMI264: sarxq
 ; BMI264: ret
   ret i64 %shl
 }
 
 define i64 @ashr64p(i64* %p, i64 %shamt) nounwind uwtable readnone {
 entry:
   %x = load i64, i64* %p
   %shl = ashr i64 %x, %shamt
 ; BMI264: ashr64p
-; BMI264: sarxq %{{.+}}, %{{.+}}, %{{.+}}
+; BMI264: sarxq %{{.+}}, ({{.+}}), %{{.+}}
 ; BMI264: ret
   ret i64 %shl
 }