[PATCH] D24108: X86: Fold tail calls into conditional branches where possible (PR26302)

Michael Kuperstein via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 1 15:47:33 PDT 2016


mkuper added a comment.

Don't know a lot about tail call handling, so the review with a grain of salt.
(The large amount of new instructions seems unfortunate, but, as above, I don't know enough about this to say whether this is avoidable...)

In https://reviews.llvm.org/D24108#531998, @maksfb wrote:

> @hans: I think it is safe to do this optimization while optimizing for size. If you do it by default it could be a performance regression if you reverse direction of the conditional branch. That's what we found out while testing on a micro benchmark. I believe it's related to the way BTB works on Intel's CPUs. The problem is that unless you are doing LTO or work at the binary level, you don't always know the new direction of the branch.


Is there a reason to believe the change in direction will cause worse performance, systematically?
Or is this just a random effect on that particular microbenchmark, and we may gain performance in other cases?


================
Comment at: include/llvm/Target/TargetInstrInfo.h:1103
@@ +1102,3 @@
+  /// Replace the conditional branch in MBB with a conditional tail call.
+  virtual void replaceBranchWithTailCall(MachineBasicBlock &MBB,
+                                         SmallVectorImpl<MachineOperand> &Cond,
----------------
Perhaps add an assert that we never get here?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3964
@@ +3963,3 @@
+  switch (BranchCond[0].getImm()) {
+    default:
+      // Can't make a conditional tail call with this condition.
----------------
What do you think about:
```
if (BranchCond[0].getImm() > X86::LAST_VALID_COND)
  return false;

```

Are there any valid condition codes you don't support?
Admittedly, the current version is safer, but I'm not sure there's a point to future-proofing this against X86 adding condition codes.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3986
@@ +3985,3 @@
+
+  if (TailCall.getOpcode() != X86::TCRETURNdi) {
+    // Only direct calls can be done with a conditional branch.
----------------
Maybe check this before checking the condition? (This would seem to be the most common reason for failure)

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4015
@@ +4014,3 @@
+
+    X86::CondCode CC = getCondFromBranchOpc(I->getOpcode());
+    assert(BranchCond.size() == 1);
----------------
Shouldn't I be contained in BranchCond?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4016
@@ +4015,3 @@
+    X86::CondCode CC = getCondFromBranchOpc(I->getOpcode());
+    assert(BranchCond.size() == 1);
+    if (CC != BranchCond[0].getImm())
----------------
Can you explain what guarantees this?
I didn't see a check in canMakeTailCallConditional().

================
Comment at: test/CodeGen/X86/conditional-tailcall.ll:20
@@ +19,3 @@
+; CHECK: jne bar
+; CHECK: encoding: [0x75,A]
+
----------------
Why do we need an encoding check?
(Probably better document this in the test itself, too.)


https://reviews.llvm.org/D24108





More information about the llvm-commits mailing list