[PATCH] Fix instruction scheduling live register tracking
chfast at gmail.com
Thu May 28 03:29:43 PDT 2015
In http://reviews.llvm.org/D9993#179812, @hfinkel wrote:
> Have you run the LLVM test suite and self-hosted with this change?
Test suite - no. What "self-hosted" actually means?
In http://reviews.llvm.org/D9993#179820, @atrick wrote:
> Thanks for the fix!
> I am generally ok with this even though I still can't prove to myself it's correct, since the existing unit tests (like add-with-overflow-128) still pass and it does fix a nasty bug.
> One quick reminder: Make sure you build for all targets before running lit tests.
I'm building all targets.
> More importantly, can you provide some explanation for why LiveRegGens cannot have a null entry at the point where you assert. My understanding is that LiveRegGens is cleared when we schedule the def. So when we're about to unschedule, the entry should be null until we set it again.
If we are at the end of a chain the Def/Gen should be cleared out around line #817. If we are not at the end of chain the other Def is expected. But I might be wrong. I suggested myself to that checking out code coverage.
> The way this is supposed to work (I think):
> - While scheduling, assign increasing "heights" to the scheduled nodes.
> - When backtracking, we know the successor that should be added back to LiveRegGens is the one with the lowest "height"
> This is almost certainly broken because the height field in the DAG node is serving dual purpose, both as a DAG height and a schedule height. One thing to try would using the ScheduleDAG node's BotReadCycle field instead of setting/checking the Height field:
> in ScheduleNodeBottom up set BotReadyCycle = CurCycle instead of SU->setHeightToAtLeast(CurCycle);
> Then in backtracking, check BotReadyCycle instead of Height. There may be other bugs in backtracking that you're hitting, but that definitely looks suspect to me.
I'll check that solution however I don't fully get it why we need to ever rewrite the Gen.
More information about the llvm-commits