Page MenuHomePhabricator

[SystemZ] Recognize INLINEASM_BR in backend
ClosedPublic

Authored by jonpa on Sep 4 2019, 4:03 AM.

Details

Reviewers
uweigand
Summary

The INLINEASM_BR (produced by asm goto) caused the backend to trigger an assertion without this patch, since that opcode was not recognized and properly handled.

This patch should handle all current cases in the backend by recognizing the opcode in getBranchInfo() and properly recording it in SystemZII::Branch. Since INLINEASM_BR is "variadic", and doesn't have any MCOperandInfo in the MCInstrDesc, I thought it best to not add any MachineOperand pointer from it in SystemZInstrInfo::getBranchInfo(). A new type SystemZII::AsmGoto is the SystemZII::BranchType given to it. I guess this could be SystemZII::Other instead, or something, since the AsmGoto type is not actually checked for anywhere.

An alternative would be to just detect the INLINEASM_BR opcode independently of getBranchInfo(), but I think that maybe would be less helpful the next time a branch needs to be analyzed - likely with getBranchInfo().

Places in backend I found that processes branches:

SystemZLongBranch:

I first thought this needed handling as well, but it turns out this is not needed since INLINEASM_BR returns false from both MI.isConditionalBranch() and MI.isUnconditionalBranch() called in SystemZLongBranch::describeTerminator().

Note: Is Large/branch-01.ll broken? I don't see any brcth in the output. Maybe just the comment is wrong? In fact it seems that the test now has Size==65204 which is less than MaxForwardRange so SystemZLongBranch does not actually run on it.

SystemZHazardRecognizer:

Needs no extra handling since branches are just checked for generally with the purpose to update the decoder group state (optimization).

SystemZMachineScheduler:

Needs to check that there is a branch Target operand, which is done by using the new isRelative() and hasMBBTarget() methods of SystemZII::Branch.

SystemZInstrInfo:
analyzeBranch() and removeBranch() needs to use hasMBBTarget().

The test asm-20.ll now runs with postra scheduling enabled (-mcpu=z14) which should make it test both analyzeBranch() and SystemZMachineScheduler().

Diff Detail

Event Timeline

jonpa created this revision.Sep 4 2019, 4:03 AM
jonpa updated this revision to Diff 218647.Sep 4 2019, 4:47 AM

Fix: Remember to check for MBB equality in SystemZPostRASchedStrategy::enterMBB()

uweigand added inline comments.Sep 4 2019, 6:03 AM
lib/Target/SystemZ/SystemZInstrInfo.cpp
1550

Please add a comment why we're not registering any branch targets.

lib/Target/SystemZ/SystemZInstrInfo.h
128

The name seems weird. If the branch target is a register operand, we have an indirect branch, not a relative branch. (In fact, relative branches are those that have an MBB target operand!)

As I'm not sure there's anything we can ever do to optimize an indirect branch, I'm not sure why we even need this.

lib/Target/SystemZ/SystemZMachineScheduler.cpp
112

Aha, here's the user. But I don't understand it -- what is supposed to be testing? Why would we want to handle indirect branches here?

jonpa updated this revision to Diff 218700.Sep 4 2019, 7:43 AM
jonpa marked 4 inline comments as done.

Updated per review.

lib/Target/SystemZ/SystemZInstrInfo.h
128

Ouch - changed the name

lib/Target/SystemZ/SystemZMachineScheduler.cpp
112

The SchedStrategy knows the previously scheduled MBB that is going to have its state copied into the current MBB being entered ends with a branch. Since decoder groups are affected differently by Taken /NT branches it first of all figures that if the target of this branch is the current MBB, then it is a taken branch.

So, as I remember it, it seemed for some reason to be good to assume that an indirect branch is typically "taken". But this check seems crude/incomplete to me. Looking at it now it seems simpler to look at the MF linear layout of blocks and just check if it is the one immediately preceding or not. Not sure why I didn't do that. Maybe they result from jump tables that have all but one of the branches taken? Are there some special rules for those branches, maybe? But I don't think that really matters that much as those branches are relatively rare, IIRC.

uweigand accepted this revision.Sep 5 2019, 2:59 AM

Anyway, given that it's already been this way, this patch LGTM. We can have a look at the indirect branch vs. scheduling question later.

lib/Target/SystemZ/SystemZMachineScheduler.cpp
112

Hmm. The SinglePredMBB must be known to be a predecessor of the current MBB, right? Is LLVM able to track predecessor relationships across indirect branches in the first place?

Do you have an example where this check triggers?

This revision is now accepted and ready to land.Sep 5 2019, 2:59 AM
jonpa closed this revision.Sep 9 2019, 4:31 AM

r371048