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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZInstrInfo.h:128
+
+  bool isRelative() { return Target != nullptr && Target->isReg(); }
+  bool hasMBBTarget() { return Target != nullptr && Target->isMBB(); }
----------------
uweigand wrote:
> 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.
Ouch - changed the name


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



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

https://reviews.llvm.org/D67151