[PATCH] D24108: X86: Fold tail calls into conditional branches where possible (PR26302)
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 6 15:40:43 PDT 2016
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
More information about the llvm-commits
mailing list