Page MenuHomePhabricator

[MSP430] Recognize Bi as an indirect branch in analyzeBranch. NFC.
ClosedPublic

Authored by foad on Sep 29 2021, 2:56 AM.

Details

Summary

Recognize Bi as an unconditional branch, just like JMP. This allows
machine verification to run after MSP430BranchSelector without failing
this assertion:

virtual bool llvm::MSP430InstrInfo::analyzeBranch(llvm::MachineBasicBlock &, llvm::MachineBasicBlock *&, llvm::MachineBasicBlock *&, SmallVectorImpl<llvm::MachineOperand> &, bool) const: Assertion `I->getOpcode() == MSP430::JCC && "Invalid conditional branch"' failed.

Note that machine verification is currently disabled after
addPreEmitPass passes because of problems on other targets, so this is
currently NFC.

Diff Detail

Event Timeline

foad created this revision.Sep 29 2021, 2:56 AM
foad requested review of this revision.Sep 29 2021, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 2:56 AM

Instead of just ignoring Bi, I think we can handle it as we do with JMP. Bi is a direct, unconditional branch like JMP, it just has longer range.
See inline comments for suggestions.

Thanks!

llvm/lib/Target/MSP430/MSP430InstrInfo.cpp
118–119

MSP430::Bi should be added here as well

193
foad added a comment.Sep 29 2021, 3:56 AM

OK, I will try your suggestions, but:

Bi is a direct, unconditional branch like JMP

then why does the Bi instruction have "isIndirectBranch = 1"?

foad updated this revision to Diff 375828.Sep 29 2021, 4:12 AM

Treat Bi as an unconditional branch.

jozefl added a subscriber: asl.Sep 29 2021, 6:35 AM

OK, I will try your suggestions, but:

Bi is a direct, unconditional branch like JMP

then why does the Bi instruction have "isIndirectBranch = 1"?

I'm not sure, looks like an oversight to me. I'll make a separate patch to fix that.

Bi corresponds to BR #imm. BR #imm is emulated as MOV @PC+, PC, which may appear to be indirect, but this is just how MSP430 deals with immediate values; the immediate value is stored in the next word after the opcode.
I'm guessing analyzeBranch cannot handle indirect branches because the destination basic block of the branch is not known when the branch target is stored in a register or memory address, but for Bi, the destination BB is known.

So this patch looks good to me, but I would like to give @asl a chance to have a look. Since this is NFC you can probably go ahead and commit it if there's no further activity after 24 hours.

Thanks,
Jozef

foad edited the summary of this revision. (Show Details)Sep 29 2021, 8:11 AM
asl accepted this revision.Sep 29 2021, 8:31 AM

Indeed, this was an oversight. Bi could be generated from an indirect branch as a result of folding or during the branch relaxation.

Interesting enough, whether we could drop branch selector pass in favour of generic BranchRelaxation pass? (MSP430 branch selector dates back to 2010 where no generic branch relaxation pass was available, the latter was created our of AArch64 one).

This revision is now accepted and ready to land.Sep 29 2021, 8:31 AM
This revision was landed with ongoing or failed builds.Sep 29 2021, 8:43 AM
This revision was automatically updated to reflect the committed changes.