[llvm-commits] [llvm] r88805 - /llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp

Mon Nov 16 10:46:16 PST 2009

On Nov 15, 2009, at 10:44 PM, Evan Cheng wrote:

> Hi Jim,
>
> Some comments for 86999 and friends:
>
> /// ReorderThumb2JumpTables - Use tbb / tbh instructions to generate  
> smaller
> /// jumptables when it's possible.
> bool ARMConstantIslands::ReorderThumb2JumpTables(MachineFunction  
> &MF) {
>
> This comment is misleading. tbb / tbh are formed later, right?

Yes. Excellent point. I'll clean that up.

>
> +  // FIXME: If it's a small block terminated by an unconditional  
> branch,
> +  // try to move it; otherwise, create a new block following the jump
> +  // table that branches back to the actual target. This is an overly
> +  // simplistic heuristic here for proof-of-concept.
>
> What do you plan as a replacement? Jump + jump seems bad for  
> performance? Why not just add a unconditional branch to replace the  
> fall through?

When we can't use a table branch instruction, we generate a jump table  
of branch instructions, so we have a jump+jump in that case. If we use  
the new fall-back situation of inserting a new jump block, then use  
the table branch instruction, we've not added any additional jumps for  
that branch target. For any forward targets of the same jump table,  
however, the second branch has been removed and performance will  
improve (this is where I suspect a good portion of our 8% gain on JSC  
is coming from).

For example, when not using TBB, we generate code like:
	adr.w	r2, #LJTI1_0_0
	add.w	r2, r2, r1, lsl #2
	mov	pc, r2
LJTI1_0_0:
	b.w LBB1_5
	b.w LBB1_19
	b.w LBB1_19
	b.w LBB1_1
	b.w LBB1_1
	b.w LBB1_7
	b.w LBB1_19
	b.w LBB1_19
	b.w LBB1_6

If we move things around and insert back-jump blocks as necessary, we  
get something like:
	tbb	[pc, r1]
LJTI1_0_0:
	.byte	(LBB1_18-LJTI1_0_0)/2
	.byte	(LBB1_20-LJTI1_0_0)/2
	.byte	(LBB1_20-LJTI1_0_0)/2
	.byte	(LBB1_17-LJTI1_0_0)/2
	.byte	(LBB1_17-LJTI1_0_0)/2
	.byte	(LBB1_16-LJTI1_0_0)/2
	.byte	(LBB1_20-LJTI1_0_0)/2
	.byte	(LBB1_20-LJTI1_0_0)/2
	.byte	(LBB1_15-LJTI1_0_0)/2

In this case, LBB1_1 was unable to be moved, but the rest were  
relocated foward. From -stats:
  3 arm-cp-islands - Number of jump table destination blocks moved
  1 arm-cp-islands - Number of jump table intermediate blocks inserted

Thus, for LBB_1, the performance is comparable before and after  
(slightly better since the adr.w and add.w instructions are no longer  
needed). For all other cases, the performance is significantly  
improved due to the direct dispatch from the TBB instruction instead  
of going through the secondary B.W in the table.

>
> +  // If the block is small and ends in an unconditional branch,  
> move it.
> +  if (Size < 50 && Cond.empty())
>
> Where is 50 coming from?

It's entirely arbitrary. I don't believe any size restriction is  
necessary, really. This was just to help get a mix of cases where the  
blocks were moved vs. when they weren't for test coverage. I'll be  
removing that bit shortly, which will also greatly simplify the side  
data structures needed (no need to track block sizes and offsets).

-Jim

>
> Evan
>
> On Nov 14, 2009, at 12:10 PM, Jim Grosbach wrote:
>
>> Author: grosbach
>> Date: Sat Nov 14 14:10:18 2009
>> New Revision: 88805
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=88805&view=rev
>> Log:
>> Cleanup flow, and only update the jump table we're analyzing when  
>> replacing a destination MBB.
>>
>> Modified:
>>   llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp
>>
>> Modified: llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp?rev=88805&r1=88804&r2=88805&view=diff
>>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp (original)
>> +++ llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp Sat Nov 14  
>> 14:10:18 2009
>> @@ -1749,7 +1749,7 @@
>>        MachineBasicBlock *NewBB =
>>          AdjustJTTargetBlockForward(MBB, MI->getParent());
>>        if (NewBB)
>> -          MJTI->ReplaceMBBInJumpTables(JTBBs[j], NewBB);
>> +          MJTI->ReplaceMBBInJumpTable(JTI, JTBBs[j], NewBB);
>>        MadeChange = true;
>>      }
>>    }
>> @@ -1772,16 +1772,16 @@
>>  int Size = BBSizes[BBI];
>>  MachineBasicBlock *TBB = 0, *FBB = 0;
>>  SmallVector<MachineOperand, 4> Cond;
>> -  // If the block terminator isn't analyzable, don't try to move  
>> the block
>> -  if (TII->AnalyzeBranch(*BB, TBB, FBB, Cond))
>> -    return NULL;
>> -
>>  // If the block is small and ends in an unconditional branch, move  
>> it.
>>  if (Size < 50 && Cond.empty()) {
>> +    // If the block terminator isn't analyzable, don't try to move  
>> the block
>> +    if (TII->AnalyzeBranch(*BB, TBB, FBB, Cond))
>> +      return NULL;
>> +
>>    MachineFunction::iterator OldPrior = prior(BB);
>>    BB->moveAfter(JTBB);
>>    OldPrior->updateTerminator();
>> -    //BB->updateTerminator();
>> +    BB->updateTerminator();
>>    ++NumJTMoved;
>>    return NULL;
>>  }
>> @@ -1792,20 +1792,22 @@
>>  MachineFunction::iterator MBBI = JTBB; ++MBBI;
>>  MF.insert(MBBI, NewBB);
>>
>> +  //MF.splice(MBBI, NewBB, NewBB);
>> +
>>  // Add an unconditional branch from NewBB to BB.
>>  // There doesn't seem to be meaningful DebugInfo available; this  
>> doesn't
>>  // correspond directly to anything in the source.
>>  assert (isThumb2 && "Adjusting for TB[BH] but not in Thumb2?");
>>  BuildMI(NewBB, DebugLoc::getUnknownLoc(), TII->get 
>> (ARM::t2B)).addMBB(BB);
>>
>> +  // Update internal data structures to account for the newly  
>> inserted MBB.
>> +  MF.RenumberBlocks(NewBB);
>> +
>>  // Update the CFG.
>>  NewBB->addSuccessor(BB);
>>  JTBB->removeSuccessor(BB);
>>  JTBB->addSuccessor(NewBB);
>>
>> -  // Update internal data structures to account for the newly  
>> inserted MBB.
>> -  MF.RenumberBlocks();
>> -
>>  // Insert a size into BBSizes to align it properly with the (newly
>>  // renumbered) block numbers.
>>  BBSizes.insert(BBSizes.begin()+NewBB->getNumber(), 0);
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>