[llvm-commits] Please review: /llvm/trunk: include/llvm/CodeGen/DFAPacketizer.h lib/CodeGen/DFAPacketizer.cpp

[Andrew Trick]

On Apr 24, 2012, at 11:43 AM, Sirish Pande <spande at codeaurora.org> wrote:

> 
> I have attached Target Independent fix required for Hexagon VLIW packetizer.
> 
> Like Andrew suggested, it does not have any warnings, make check-all passes all test cases for *all* targets.
> 
> I have also incorporated Andrew's suggestions like moving exitRegion after packetization, using finishBlock and using map mi to su functionality.
> 
> Please review. And, if there are any questions or comments, please let me know.

Thanks for copying me.

 I don't understand why you need to call initSUnits. That's supposed to be a helper for buildSchedGraph. Any reason you can't use the MISUnits map that it creates?

If you can just remove that call and everything works, then go ahead and commit.

-Andy

> 
> Here's  the result of make check-all:
> 
> -- Testing: 10489 tests, 8 threads --
> Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
> Testing Time: 36.53s
>  Expected Passes    : 10400
>  Expected Failures  : 76
>  Unsupported Tests  : 13
> 
> 
> Sirish
> 
>> 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
>> 
> 
> 
> -- 
> Qualcomm Innovation Center, Inc is a member of Code Aurora Forum
> 
> <HexagonPacketizerTargetIndependentFix.patch>