[llvm-commits] Please review: Support for Hexagon new value jump

[Andrew Trick]

On Apr 25, 2012, at 10:13 PM, Sirish Pande <spande at codeaurora.org> wrote:
> Hi,
> 
> This is the support hexagon new value jump architectural feature.
> 
> This patch does not give warnings and passes all test cases with make check-all.

Sirish,

Given that this patch didn't build from trunk at the time you sent it, I wouldn't expect reviewers to pay attention to it. It does build for me after applying a few other patches in the right order.

Since this change is entirely in the Hexagon target, I'm ok with it going in unless someone else objects. One thing you should keep in mind though as you upsteam gobs of messy target code: this code must be maintained by contributors who care about Hexagon. The more assumptions you make about other codegen passes, the heavier the burden will be. If the target isn't kept up to speed, it will gradually be disabled and eventually removed. So you should seriously think about *not* upstreaming experimental or optional passes that other Hexagon users don't need. In fact, are there other Hexagon users? Do you really need to upstream everything?

Suggestions:

Although you have some comments at the top of the file, this pass could be explained better. What are the phase ordering issues? Why do you run this so late? If you want physreg liveness, why don't you run closer to regalloc and use the block's physreg live-in sets while they're still valid?

+       // I am doing this only because LLVM does not provide LiveOut
+       // at the BB level.

I don't understand this comment. LiveIn/LiveOut can be inferred from each other. It's valid after regalloc until someone invalidates it.

If you can rely on the block livein/out sets, then it would be preferrable if you could ignore kill flags and compute local liveness yourself. You wouldn't need to update kill flags, just conservatively clear them.

Something I think you should address, if not now, then when you expect the design to change significantly:

+ bool HexagonNewValueJump::runOnMachineFunction

This function should simply be rewritten. It has 310 lines, 8 levels of indentation, and 17 "control" variables. You may want a class here. Or split up the iteration over instructions so it can be done in more than one place. I'm certainly not going to pretend to understand much of it.

-Andy