[llvm] r183174 - Fix a defect in code-layout pass, improving Benchmarks/Olden/em3d/em3d by about 30%

[Shuxin Yang]

 >Also, you *must* provide test cases for this kind of change. It is far 
too subtle otherwise, and will just break in the future.
I wish I could, but it is really difficult  to fabricate one.

>
>     rdar://13966341
>
>     Modified:
>         llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
>
>     Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
>     URL:
>     http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=183174&r1=183173&r2=183174&view=diff
>     ==============================================================================
>     --- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
>     +++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Mon Jun  3
>     20:00:57 2013
>     @@ -991,6 +991,28 @@ void MachineBlockPlacement::buildCFGChai
>          Cond.clear();
>          MachineBasicBlock *TBB = 0, *FBB = 0; // For AnalyzeBranch.
>          if (!TII->AnalyzeBranch(*PrevBB, TBB, FBB, Cond)) {
>     +      // The "PrevBB" is not yet updated to reflect current code
>     layout, so,
>     +      //   o. it may fall-through to a block without explict
>     "goto" instruction
>     +      //      before layout, and no longer fall-through it after
>     layout; or
>     +      //   o. just opposite.
>
>
> Yes, and my memory of AnalyzeBranch from when I wrote this code is 
> that it explicitly had logic (at least for x86) to handle these cases. 
> We need it to handle them because updateTerminator (the routine which 
> fixes the terminator to not have the broken properties you observe in 
> PrevBB) can only be called when the branch is analyzable. See the 
> FIXME comment just a few lines up.
The problem is we cannot blindly call updateTerminator(); it will 
complains if the branch is not analyzable.

>     +      //
>     +      // AnalyzeBranch() may return erroneous value for FBB when
>     these two
>     +      // situations take place. For the first scenario FBB is
>     mistakenly set
>     +      // NULL; for the 2nd scenario, the FBB, which is expected
>     to be NULL,
>     +      // is mistakenly pointing to "*BI".
>
>
> This is either a bug in AnalyzeBranch, or a bug in us calling 
> AnalyzeBranch. I suspect it is a bug in one target's implementation of 
> AnalyzeBranch, but the API and behavior for that routine is so 
> *incredibly* convoluted and poorly specified that I could believe 
> either one. However, the fix should not be here! Instead we should fix 
> updateTerminator and AnalyzeBranch to work in a more sane way.
This is the bug in the placement pass -- AnalyzeBranch() is fed with 
stale parameter.
That is why the change is at code layout side.

You may argue AnalyzeBranch() should return non-NULL two branch target 
no matter if it has
explicit "goto" instruction or not. I think it is better interface, 
however,  I don't feel like to
change AnalyzeBranch() this way, as we need to update all its callers.
>
> Let me see if I understand the problem correctly:
>
>     +      //
>     +      bool needUpdateBr = true;
>     +      if (!Cond.empty() && (!FBB || FBB == *BI)) {
>     +        PrevBB->updateTerminator();
>
>
> OK, so the goal is that:
>
> 1) If the branch is analyzable, but
> 2) We get garbage back from the analysis, then
> 3) We update the terminators, and
> 4) Re-analyze the branch in order to actually make the terminator 
> reversal decision correctly, and then
> 5) Re-update the terminator?
>
Yes. It can be simplified into following if we can blindly call 
updateTerminator() without triggering assertion.
1) updateTerminator() blindly
2) analyzeBranch().


> If I've understood the problem correctly, then I think there are two 
> possible correct solutions, but both look somewhat different from this:
>
> A) We could solve in essentially the same way you have here, but more 
> cleanly. This would be done by teaching updateTerminator to work even 
> in the cases where AnalyzeBranch actually fails, so that rather than 
> asserting, it simply bails out. With this change, we could write much 
> simpler code:
>
> PrevBB->updateTerminator();
>
> if (TII->AnalyzeBranch(...)) {
>   if (/* existing code to check for a profitable reversal case */) {
>     // reverse the branch and condition
>     PrevBB->updateTerminator(); // because we monkeyed with it
>   }
> }
>
> Does that make sense? It seems much more clear than the current code 
> sequence.
That was exactly my 1st fix, and later on I realize updateTerminator() 
cannot be blindly called because
assertion will be triggered if the branch is not analyzable.  I don't 
want to remove the assertion as it make sense
to me.

My current fix is slightly different from this one. It just let 
updateTerminator() guarded by AnalyzeBranch(),
I add extra few lines of code to make it efficient --- AnalyzeBranch() 
and updateTerminator() dosen't seem to be
very expensive; one cycle saved, one cycle earned.

if (TII->AnalyzeBranch(PrevBB) {
      PrevBB->updateTerminator().
      TII->AnalyzeBranch(PrevBB); // analyze one more time.
      ...
     reverse etc ...
     ...
}


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130604/4e8bd8e8/attachment.html>