[llvm-commits] Please review: Hexagon Packetizer Support

[Andrew Trick]

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