[llvm-commits] Please review: Hexagon Packetizer Support

Sirish Pande spande at codeaurora.org
Tue May 1 07:52:34 PDT 2012


On 4/30/2012 10:23 PM, Andrew Trick wrote:
> On Apr 25, 2012, at 10:12 PM, Sirish Pande<spande at codeaurora.org>  wrote:
>
>> Hi,
>>
>> This is support for hexagon target dependent vliw packetizer.
>>
>> This patch does not give warnings and passes all test cases with make check-all.
>>
>> Sirish
>>
>> -- 
>> Qualcomm Innovation Center, Inc is a member of Code Aurora Forum
>>
>> <HexagonPacketizerTargetCode.patch>
> One problem with this patch, and the others you posted last week, is that it can't be applied independently. I understand you want patches reviewed as a group before going through another iteration on your side. I also understand that reviewers on this list want each area functionality reviewed as a separate email thread. If you post a patch that depends on other uncommitted patches, please be explicit about those dependencies. It needs to be obvious to a reviewer how to build your patch without searching the mailing list and guessing the order. I don't mind if you include the dependent patches as long as it's clear which patch is being reviewed. Or you can refer to message URLs.
>
> Another problem is that the patch won't build because you left out the target description changes. I found those buried in another patch earlier, but as you can tell by now, I'd rather not search message threads.
>
> The giant property tables in HexagonInstrInfo.cpp and HexagonVLIWPacketizer.cpp don't look good to me. Why can't you use TSFlags as you've done for other properties?
>
> As long as you have a good reason for that, I think you can commit this... after checking in the target-independent packetizer and the .td file changes of course.
>
> Next time you submit a patch, you can help the review by taking care of
> a few things first...
>
> In HexagonAsmPrinter.cpp: Some of the comments are stale or incorrect. Your patch changes assert lines that were previously nicely indented to very strange indentation.
>
> In HexagonISelDAGToDAG.cpp: whitespace changes mixed with code changes
>
> In HexagonVLIWPacketizer: 80-col violations
>
> I expect you'll address these in the next round of changes.
>
> -Andy

Andy,

First of all, thanks a lot for taking your time and looking at the 
patches. Given that HexagonVLIWPacketizer is the the pre-req for all the 
other Hexagon patches, I should have been more explicit on that. Once 
HexagonVLIWPacketizer is upstreamed, all the other patches can be 
totally independent patches. And, I will make sure that it will be 
upstreamed that way. So, for this patch, I will address your comments, 
and re-post.

Once again, thanks for taking your time in reviewing this patch.

Sirish

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




More information about the llvm-commits mailing list