[PATCH] D9067: LiveIntervalAnalysis: Support moving of subregister defs in handleMove

[Andrew Trick via llvm-commits]

atrick added a comment.

I'm just commenting here, not blocking the commit.

After staring for a long time, I basically understand what you're fixing. 
But I can't review your changes without critiquing the code as a whole--it's too hard to reason about.

First off--and I realize you're not in a position to fix this--the API is not general enough. A repairIntervalsInRange-like API is what's needed. The design should strive to reuse the interval construction logic, rather than stringing together a number of special cases. I imagine scanning an range of instructions rebuilding live intervals along the way, with a filter that ignores "untouched" virtual registers.

Given that we're stuck at present with a string of special cases, it's good that each case to be handled is listed in the function comment. But those comments do not specify precisely the conditions that need to be checked and aren't obvisouly associated with the implementation in the function body. At a particular point in the function, it isn't clear what assumptions are being made.

For example, these are the kind of questions I have when examining handleMoveDown:

- What conditions are associated with cases 1-6. How can we reason about the completeness?

- Where exactly are cases 1-6 handled inside the function body?

- When subregisters are involved how to you know whether the moved instruction is a use or def?

- How do you know the extended live interval should end at RegSlot? What about early clobber, dead register?

The new code that directly updates start, end, valno is too low-level in the context of handleMoveDown.


Repository:
  rL LLVM

http://reviews.llvm.org/D9067