[llvm-commits] [llvm] r155364 - in /llvm/trunk: include/llvm/CodeGen/DFAPacketizer.h lib/CodeGen/DFAPacketizer.cpp

[Sirish Pande]

On 4/23/2012 3:26 PM, Andrew Trick wrote:
>
> On Apr 23, 2012, at 11:55 AM, Sirish Pande <spande at codeaurora.org 
> <mailto:spande at codeaurora.org>> wrote:
>
>> On 4/23/2012 1:29 PM, Chandler Carruth wrote:
>>> On Mon, Apr 23, 2012 at 11:09 AM, Sirish Pande 
>>> <spande at codeaurora.org <mailto:spande at codeaurora.org>> wrote:
>>>
>>>     There were only two reviews:
>>>
>>>     1. Eric C asked for test cases, and that test cases be
>>>     associated with the patch.
>>>     2. Tom Stellard had requested to split the vliw packetizer patch.
>>>
>>>     Both have been done.
>>>
>>>
>>> But the review isn't finished yet. You need to wait for the 
>>> reviewers to actually sign off on the change before submitting. The 
>>> two comments you got were what people needed changed based on their 
>>> first look, that doesn't mean the code is ready to be submitted yet.
>>>
>>> This patch is particularly problematic because it is not just 
>>> changing the Hexagon, it's changing a core part of the codegen 
>>> layer. Those changes need proper review before being committed, and 
>>> that hasn't happened yet.
>>>
>>> I've reverted this patch until the review finishes because the 
>>> patches requiring it had to be reverted anyways.
>>>
>>> I reverted the hexagon patches because your own regression tests 
>>> failed for all of the patches in the series.
>>>
>>> This gets the tree back into both a reviewed and test-passing state. 
>>> Let's keep it that way. =]
>>
>> Fair enough. I will wait for the reviewers for sign off.
>
> This target-independent code looks ok to commit. (I haven't looked at 
> your target-specific patches yet.)
>
> As usual, under the following conditions
> - no build warnings
> - try to avoid significant increase in build times
> - make check-all passes when configured for *all* targets
>   (this should apply to everyone committing target sensitive code, not 
> just you!)
> - Address buildbot failures that occured last time
>
> Chandler mentioned that some bots showed failures, so please look into 
> those and don't commit until you think you've fixed them.
>
> A couple comments...
>
> +  VLIWScheduler->exitRegion();
>
> I'm not sure why you would do this *before* packetizing. It's ok if it 
> works, just a little misleading. You should probably add a call to 
> VLIWScheduler->finishBlock() somewhere.

Thanks for pointing out about the finishBlock(). I will add 
finishBlock() at the end of PacketizeMI function.

You probably know that we are getting the dependence information from 
the scheduler. Once we have the dependence info, along with the DFA 
resource manager, we can packetize the way our architecture prefers.  We 
are not using the scheduler to *schedule* instructions during 
packetization. Hence, we have enterRegion() and exitRegion() around 
schedule. Esssentially, this logic is borrowed from around line 200 of 
MachineScheduler.cpp
>
> +  // Generate MI -> SU map.
> +  //std::map <MachineInstr*, SUnit*> MIToSUnit;
> +  MIToSUnit.clear();
> +  for (unsigned i = 0, e = VLIWScheduler->SUnits.size(); i != e; ++i) {
> +    SUnit *SU = &VLIWScheduler->SUnits[i];
> +    MIToSUnit[SU->getInstr()] = SU;
> +  }
>
> I added ScheduleDAGInstrs::MISUnitMap so you wouldn't need your own. I 
> haven't tested it with a PostRA scheduler, but would like to know if 
> it doesn't work.

Have you checked this functionality in? I don't see it.
>
> Thanks!
>
> -Andy
>


-- 
Qualcomm Innovation Center, Inc is a member of Code Aurora Forum

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120424/872c4983/attachment.html>