[PATCH] D57718: [PPC] Adjust the computed branch offset for the possible shorter distance

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think I misunderstood the original problem this is solving. What I thought it was solving is any inline asm or alignment directives that appear prior to the branch or its target (whichever comes last in the function). However, after looking through the code, it appears that the problem is only if the inline asm or alignment directive appears prior to the branch or its target (**whichever comes first in the function**).
It would probably be good to explain why it isn't a problem if the block that throws off the computation is between the branch and its target.



================
Comment at: lib/Target/PowerPC/PPCBranchSelector.cpp:80
+  // Minimum block number which has imprecise instruction address.
+  int MinImpreciseBlock = -1;
 
----------------
It seems that this might be better named `FirstImpreciseBlock`.


================
Comment at: lib/Target/PowerPC/PPCBranchSelector.cpp:200
+          BranchSize += BlockSizes[DestBlock].first;
+          for (unsigned i = DestBlock+1, e = MBB.getNumber(); i < e; ++i) {
             BranchSize += BlockSizes[i].first;
----------------
I don't actually follow why we don't count the size of the destination block since we're branching to the top of it. Could you add a comment explaining why?


================
Comment at: lib/Target/PowerPC/PPCBranchSelector.cpp:207
+          NeedExtraAdjustment = (MinImpreciseBlock >= 0) &&
+                                (DestBlock >= MinImpreciseBlock);
         } else {
----------------
I also don't understand this... How about if the first "imprecise block" is between the destination block and the branch block? Wouldn't we still possibly need the extra adjustment? Namely, shouldn't this say something like `MBB.getNumber() >= MinImpreciseBlock`?


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

https://reviews.llvm.org/D57718