[X86]: Don't set a regmask on conditional tail calls (PR31257)

[hans]

Summary

The regmask would cause analyses to think the conditional tail call clobbers registers. Specifically, Machine Copy Propagation would remove a copy instruction to %ecx before the tail call (see PR).

Since the tail call is conditional, registers are only clobbered if the condition is met, in which case control leaves the function so it doesn't matter. Therefore, model the instruction as not clobbering any registers.

[hans]

Ping?

[mkuper]

Sorry for the lag, Hans.

I thought about this a bit more, and I'm not sure I'm a fan of this, even though I basically suggested something similar on Friday.

Two issues:

1. I'm still not sure this won't have any unintended consequences.
2. Having BuildMI return an instruction that requires you to remove an operand for correctness seems rather inelegant. I agree that this is probably "safer" than removing IsCall, and that is probably the only code that will ever construct a TCRETURNdi64cc MI, but, still...

I don't have any good ideas, though.

[hans]

In D29511#668161, @mkuper wrote:

Sorry for the lag, Hans.

I thought about this a bit more, and I'm not sure I'm a fan of this, even though I basically suggested something similar on Friday.

Two issues:

1. I'm still not sure this won't have any unintended consequences.
2. Having BuildMI return an instruction that requires you to remove an operand for correctness seems rather inelegant. I agree that this is probably "safer" than removing IsCall, and that is probably the only code that will ever construct a TCRETURNdi64cc MI, but, still...

I don't have any good ideas, though.

I agree with 2), that code is clumsy at best. I think what happened is that we regressed this in r281223: we shouldn't have copied the regmask in the first place. We don't have to remove an operand for correctness, it was incorrect to add it. I'm uploading a new patch that does this.

As for constructing TCRETURNdi64cc elsewhere, well they shouldn't put regmask on it either.

For 1) well, that's the case with a lot of code. I'm feeling pretty good about this change though. (I don't think removing isCall would actually work, as the regmask is copied over from the unconditional tail call. I suppose we could remove isCall from that, but that doesn't feel right at all.)

[hans]

Don't copy the regmask in teh first place.

[mkuper]

What's special about this code (and why I'm reluctant for it to go in) is that we're "lying" :-) That is, we're pretending an instruction that may clobber registers doesn't. That kind of stuff tends to be brittle.
Also, it's inconsistent - if the reasoning is that a TCRETURN doesn't need to provide a regmask (because which registers a return "clobbers" doesn't matter), then I guess we should fix this by never attaching a mask to any TCRETURN. Then you also won't have the problem of copying it here, since the original TCRETURN won't have one either.

Re 2 - I didn't notice where the regmask was actually coming from! Yes, this makes much more sense. But see above.

[hans]

I've reverted the conditional tail calls in r294348 to fix trunk and the branch while we figure out the proper solution. I've CC'd Quentin on the bug so we can discuss it there.

lib/Target/X86/X86InstrInfo.cpp

void X86InstrInfo::replaceBranchWithTailCall(
 
   unsigned Opc = TailCall.getOpcode() == X86::TCRETURNdi ? X86::TCRETURNdicc
                                                          : X86::TCRETURNdi64cc;
 
   auto MIB = BuildMI(MBB, I, MBB.findDebugLoc(I), get(Opc));
   MIB->addOperand(TailCall.getOperand(0)); // Destination.
   MIB.addImm(0); // Stack offset (not used).
   MIB->addOperand(BranchCond[0]); // Condition.
-  MIB.copyImplicitOps(TailCall); // Regmask and (imp-used) parameters.
+
+  // Pick up implicitly used registers (call parameters).
+  for (const MachineOperand &MO : TailCall.implicit_operands()) {
+    if (MO.isReg())
+      MIB->addOperand(MO);
+  }
+
+  // Note: We must not copy the regmask from TailCall, as the conditional tail
+  // call does not clobber any registers unless the branch is taken, in which
+  // case control leaves the function so it doesn't matter.
 
   I->eraseFromParent();
 }
 
 // Given a MBB and its TBB, find the FBB which was a fallthrough MBB (it may
 // not be a fallthrough MBB now due to layout changes). Return nullptr if the
 // fallthrough MBB cannot be identified.
 static MachineBasicBlock *getFallThroughMBB(MachineBasicBlock *MBB,

test/CodeGen/X86/conditional-tailcall.ll

-; RUN: llc < %s -mtriple=i686-linux -show-mc-encoding | FileCheck %s
-; RUN: llc < %s -mtriple=x86_64-linux -show-mc-encoding | FileCheck %s
+; RUN: llc < %s -mtriple=i686-linux -show-mc-encoding | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK32
+; RUN: llc < %s -mtriple=x86_64-linux -show-mc-encoding | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK64
 
 declare void @foo()
 declare void @bar()
 
 define void @f(i32 %x, i32 %y) optsize {
 entry:
   %p = icmp eq i32 %x, %y
   br i1 %p, label %bb1, label %bb2

 
 ; Make sure machine block placement isn't confused by the conditional tail call,
 ; but sees that it can fall through to the next block.
 ; CHECK-LABEL: BlockPlacementTest
 ; CHECK: je baz
 ; CHECK-NOT: xor
 ; CHECK: ret
 }
+
+
+
+%"class.std::basic_string" = type { %"struct.std::basic_string<char, std::char_traits<char>, std::allocator<char> >::_Alloc_hider" }
+%"struct.std::basic_string<char, std::char_traits<char>, std::allocator<char> >::_Alloc_hider" = type { i8* }
+declare zeroext i1 @_Z20isValidIntegerSuffixN9__gnu_cxx17__normal_iteratorIPKcSsEES3_(i8*, i8*)
+
+define zeroext i1 @pr31257(%"class.std::basic_string"* nocapture readonly dereferenceable(8) %s) minsize {
+; CHECK-LABEL: pr31257
+entry:
+  %_M_p.i.i = getelementptr inbounds %"class.std::basic_string", %"class.std::basic_string"* %s, i64 0, i32 0, i32 0
+  %0 = load i8*, i8** %_M_p.i.i, align 8
+  %arrayidx.i.i.i54 = getelementptr inbounds i8, i8* %0, i64 -24
+  %_M_length.i.i55 = bitcast i8* %arrayidx.i.i.i54 to i64*
+  %1 = load i64, i64* %_M_length.i.i55, align 8
+  %add.ptr.i56 = getelementptr inbounds i8, i8* %0, i64 %1
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %it.sroa.0.0 = phi i8* [ %0, %entry ], [ %incdec.ptr.i, %for.inc ]
+  %state.0 = phi i32 [ 0, %entry ], [ %state.1, %for.inc ]
+  %cmp.i = icmp eq i8* %it.sroa.0.0, %add.ptr.i56
+  br i1 %cmp.i, label %5, label %for.body
+
+for.body:                                         ; preds = %for.cond
+  switch i32 %state.0, label %for.inc [
+    i32 0, label %sw.bb
+    i32 1, label %sw.bb14
+    i32 2, label %sw.bb22
+  ]
+
+sw.bb:                                            ; preds = %for.body
+  %2 = load i8, i8* %it.sroa.0.0, align 1
+  switch i8 %2, label %if.else [
+    i8 43, label %for.inc
+    i8 45, label %for.inc
+  ]
+
+if.else:                                          ; preds = %sw.bb
+  %conv9 = zext i8 %2 to i32
+  %isdigittmp45 = add nsw i32 %conv9, -48
+  %isdigit46 = icmp ult i32 %isdigittmp45, 10
+  br i1 %isdigit46, label %for.inc, label %cleanup.thread.loopexit
+
+sw.bb14:                                          ; preds = %for.body
+  %3 = load i8, i8* %it.sroa.0.0, align 1
+  %conv16 = zext i8 %3 to i32
+  %isdigittmp43 = add nsw i32 %conv16, -48
+  %isdigit44 = icmp ult i32 %isdigittmp43, 10
+  br i1 %isdigit44, label %for.inc, label %cleanup.thread.loopexit
+
+sw.bb22:                                          ; preds = %for.body
+  %4 = load i8, i8* %it.sroa.0.0, align 1
+  %conv24 = zext i8 %4 to i32
+  %isdigittmp = add nsw i32 %conv24, -48
+  %isdigit = icmp ult i32 %isdigittmp, 10
+  br i1 %isdigit, label %for.inc, label %if.else28
+
+; Make sure Machine Copy Propagation doesn't delete the mov to %ecx becaue it
+; thinks the conditional tail call clobbers it.
+; CHECK64-LABEL: .LBB2_11:
+; CHECK64:       movzbl  (%rdi), %ecx
+; CHECK64-NEXT:  addl    $-48, %ecx
+; CHECK64-NEXT:  cmpl    $10, %ecx
+; CHECK64-NEXT:  movl    %r9d, %ecx
+; CHECK64-NEXT:  jae     _Z20isValidIntegerSuffixN9__gnu_cxx17__normal_iteratorIPKcSsEE
+
+if.else28:                                        ; preds = %sw.bb22
+  %call34 = tail call zeroext i1 @_Z20isValidIntegerSuffixN9__gnu_cxx17__normal_iteratorIPKcSsEES3_(i8* nonnull %it.sroa.0.0, i8* %add.ptr.i56)
+  br label %cleanup.thread
+
+for.inc:                                          ; preds = %sw.bb, %sw.bb, %sw.bb22, %sw.bb14, %if.else, %for.body
+  %state.1 = phi i32 [ %state.0, %for.body ], [ 1, %sw.bb ], [ 2, %if.else ], [ 2, %sw.bb14 ], [ 2, %sw.bb22 ], [ 1, %sw.bb ]
+  %incdec.ptr.i = getelementptr inbounds i8, i8* %it.sroa.0.0, i64 1
+  br label %for.cond
+
+; <label>:5:                                      ; preds = %for.cond
+  %cmp37 = icmp eq i32 %state.0, 2
+  br label %cleanup.thread
+
+cleanup.thread.loopexit:                          ; preds = %if.else, %sw.bb14
+  br label %cleanup.thread
+
+cleanup.thread:                                   ; preds = %cleanup.thread.loopexit, %if.else28, %5
+  %6 = phi i1 [ %cmp37, %5 ], [ %call34, %if.else28 ], [ false, %cleanup.thread.loopexit ]
+  ret i1 %6
+}

test/CodeGen/X86/tail-call-conditional.mir

body:             |
 
   ; CHECK: bb.1:
   ; CHECK-NEXT: successors: %bb.2({{[^)]+}}){{$}}
   ; CHECK-NEXT: liveins: %rax, %rsi
   ; CHECK-NEXT: {{^  $}}
   ; CHECK-NEXT: %rdi = COPY %rsi
   ; CHECK-NEXT: %rsi = COPY %rax
   ; CHECK-NEXT: CMP64ri8 %rax, 9, implicit-def %eflags
-  ; CHECK-NEXT: TCRETURNdi64cc @f1, 0, 3, csr_64, implicit %rsp, implicit %eflags, implicit %rsp, implicit %rdi, implicit %rsi
+  ; CHECK-NEXT: TCRETURNdi64cc @f1, 0, 3, implicit %rsp, implicit %eflags, implicit %rsp, implicit %rdi, implicit %rsi
 
   bb.1:
     successors: %bb.2, %bb.3
     liveins: %rax, %rsi
 
     CMP64ri8 %rax, 9, implicit-def %eflags
     JA_1 %bb.3, implicit %eflags
     JMP_1 %bb.2