[PATCH] D67151: [SystemZ] Recognize INLINEASM_BR in backend

Thu Sep 5 02:59:35 PDT 2019

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

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.

================
Comment at: lib/Target/SystemZ/SystemZMachineScheduler.cpp:112
+                        (TII->getBranchInfo(*I).isRelative() ||
+                         TII->getBranchInfo(*I).getMBBTarget() == MBB));
     HazardRec->emitInstruction(&*I, TakenBranch);
----------------
jonpa wrote:
> uweigand wrote:
> > 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?
> 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.
> 
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?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67151/new/

https://reviews.llvm.org/D67151