[PATCH] D19486: Keep dead inst copy from being deleted only when the inst is rematerializable

[Wei Mi via llvm-commits]

On Mon, Apr 25, 2016 at 3:12 PM, Wei Mi <wmi at google.com> wrote:
> wmi added a comment.
>
>> Additing the alias analysis seem unreleated to me.
>
>>  Could it be a separate patch?
>
>
> Adding alias analysis is because isTriviallyReMaterializable will use it.  I am not sure whether separating the alias analysis part will cause unused-var-warning.
>
>> Could you add a test case?
>
>
> Ok, will do that.
>
> Thanks,
> Wei.
>
>
> ================
> Comment at: lib/CodeGen/LiveRangeEdit.cpp:331
> @@ -330,1 +330,3 @@
> +    // allocations of the func are done.
> +    if (isOrigDef && DeadRemats && TII.isTriviallyReMaterializable(MI, AA)) {
>        LiveInterval &NewLI = createEmptyIntervalFrom(Dest);
> ----------------
> qcolombet wrote:
>> Don't we already check that MI is triviallyReMaterializable before trying to rematerialize?
>> I.e., this is just an optimization to keep DeadRemats small, not the actual fix, right?
>>
>> In other words, if this is the proposed fix, I think I miss something!
> This patch is to fix the problems by keeping DeadRemats small.
>
> For instruction like vreg1 = vreg2 + 3 which is not triviallyReMaterializable because it has a vreg use on rhs, if it is dead, previously we still put it in DeadRemats and fail to update the live interval after we delete it in postOptimization. This will cause verifier error.
>
> Now we reduce the DeadRemats set to dead instructions which will truly possible to be used in rematerialization, we don't have to worry live interval update because instructions in DeadRemats won't have vreg use. vreg dest is a dummy register and its live interval has been properly set.
>
> In other words, keep instructions in DeadRemats to a limited form enforced by triviallyReMaterializable, we can simplify some live interval update hassle and keep the machine verifier silent.
>
>

Quentin, I think your question makes sense. Previously I thought
eliminateDeadDef will not only delete instruction after the uses of
its dest reg are fully rematerialized, but also transitively delete
instructions if their dest regs are only used in the dead instruction.
But I missed the fact if triviallyReMaterializable limits that
rematerializable instruction doesn't have vreg use on rhs,
eliminateDeadDef will not see such transitively dead instructions.

I think that is why existing testings cannot trigger this error. I
just added some assertions and tried to catch a testcase in llvm
testsuite but failed. Mikael also mentioned they caught the errors
after run a lot of out-of-tree testings.

I think I had better ask Mikael for the original error testcases.

Thanks,
Wei.