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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Thank you for providing the testcase, I think it corrects my understanding.



================
Comment at: lib/Target/PowerPC/PPCBranchSelector.cpp:214
+          //    ...
+          //   bne Far      100            10c
+          //   .p2align 4
----------------
jsji wrote:
> Carrot wrote:
> > jsji wrote:
> > > Looks like to me that the root cause here is that 
> > > 1. we **may have conservative (larger) MBBStartOffset** due to inline asm or alignment,  (eg: `100` -> `10c`)
> > > 2.  we are **subtracting MBBStartOffset** here  (eg: `0x8108 - 0x10c`)
> > > 
> > > So instead of getting larger `BranchSize`, subtracting may lead us to getting smaller `BranchSize`.
> > > 
> > > For other blocks, even if we may still have conservative (larger) size, it should be OK, as we are adding them towards `BranchSize`.
> > > 
> > > 
> > > So, shouldn't we just need to adjust the value of `MBBStartOffset` , checking inline asm & alignment when calculating it in previous loop?
> > The root cause in your understanding is correct!
> > 
> > The inline asm size is handled by 
> >   MBBStartOffset += TII->getInstSizeInBytes(*I);
> > The BB alignment is already handled when computing BlockSizes.
> > 
> > Here the problem is the computed MBBStartOffset can't be precise, and we can't tell what's the difference between its value and the actual address. For the BranchSize, if it is larger than actual offset, it is safe for us. If it is smaller than actual branch offset, the max delta is 
> >       (1 << MaxAlign) - 4
> > 
> Thanks. 
> Can we tell whether `MBBStartOffset` is precise or not by checking whether we meet inline asm & alignment?
> And only apply the delta when we are sure they are not precise?
> 
> Also is it possible that `MBBStartOffset` is smaller than ` (1 << MaxAlign) -4 `?
> 
> And can we add some comments about why the max delta is 
> ` (1 << MaxAlign) -4 ` ?
> 
> 
Actually, after seeing your new comments and the testcase. I think my initial understanding was wrong.
The root cause of the estimate gap is not due to that we are `subtracting MBBStartOffset `.

The root cause looks like to me now:

1.  We may have conservative (larger) `BlockSizes` due to inline asm .
      eg: In your example, we add 4 extra bytes due to label in inline asm
2.  We may get **smaller than actual alignment adjustment** due to that inaccurate `Offset`. 
      eg: In your example,  we get offset 0, as we though the offset will be `32`, however, it is actually `28`, 
            so , we should get an adjustment of `12` here! So that is why we are actually `-12` to real value here!

So looks like to me,  after an inaccurate inline asm BB, all the alignment adjustment of a BB **may get a smaller than actual value (negative!).** , and since we are adding all these toward `BranchSize`, I think this problem will  **not only happen in forward branch, but also in backwards branch.**:

So maybe we should fix this in `GetAlignmentAdjustment` to let is be conservative when a BB contains inline asm?



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

https://reviews.llvm.org/D57718