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

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks very much for the review. New version uploaded.


================
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.
----------------
mkuper wrote:
> 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.
Thanks, that works great.

I was worried about special condition codes like COND_NE_OR_P, but LAST_VALID_COND handles that.

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

BranchCond is what we got from analyzeBranch() before. We don't really have a handle to the actual branch instruction, which is why we're searching for it here.

================
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())
----------------
mkuper wrote:
> Can you explain what guarantees this?
> I didn't see a check in canMakeTailCallConditional().
BranchCond originally comes from X86InstrInfo::analyzeBranch(), and that one only puts one element in it.

I'll add the same assert to canMakeTailCallConditional().


https://reviews.llvm.org/D24108