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

[Evan Cheng]

On Nov 16, 2009, at 10:46 AM, Jim Grosbach wrote:

> 
> 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

That's going to be changed. It's currently this way due to a Darwin linker issue.

> 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.

I believe this is not going to make jump table performance worse. The question is whether it can be better. If we simply move LBB1_1 and replace its fallthrough edge with an unconditional edge, would that be better?
> 
> 
>> 
>> +  // 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).

Ok.

Evan

> 
> -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
>> 
>