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

[Andrew Trick]

On Apr 23, 2012, at 11:55 AM, Sirish Pande <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> 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.

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

Thanks!

-Andy

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