Page MenuHomePhabricator

[BranchFolder] Drop kill/dead flags if they aren't present in all merged instructions
Needs ReviewPublic

Authored by uabelho on Aug 21 2018, 1:37 AM.

Details

Summary

Just like we already do with undef flags, we need to drop kill flags if
they aren't present in all merged instructions. Otherwise we would merge

<instr1> $r0
<instr2> killed $r0

and

<instr1> killed $r0
<instr2> undef $r0

into

<instr1> killed $r0
<instr2> $r0

and then the verifier would complain about $r0 not being live at <instr2>.

Similarly, we need to drop dead flags if they aren't present in all
merged instructions. Otherwise we would merge

dead $r0 = <instr1>
<instr2> undef $r0

and

$r0 = <instr1>
<instr2> $r0

into

dead $r0 = <instr1>
<instr2> $r0

and then the verifier would complain about $r0 not being live at <instr2>.

Diff Detail

Event Timeline

uabelho created this revision.Aug 21 2018, 1:37 AM

Found the problem when testing my out of tree target, but it's exposed also with the attached testcase.

Does the fix make sense or should we do something else?

uabelho updated this revision to Diff 197926.May 3 2019, 1:07 AM

Rebased, fixed a spelling mistake.

Anyone has an opinion about this?

uabelho updated this revision to Diff 234285.Dec 17 2019, 5:59 AM
uabelho retitled this revision from [BranchFolder] Drop kill flags if they aren't present in all merged instructions to [BranchFolder] Drop kill/dead flags if they aren't present in all merged instructions.
uabelho edited the summary of this revision. (Show Details)

Rebased, updated to drop dead flags as well.

Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 5:59 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

I am a little curious as to how these differences in flags arise. I can understand that a kill flag can be seen as an optimization and is optional. The same would go for a dead flag. But I am not sure about an undef flag... If it is present in one case but not in the other, the two instructions do not seem equivalent, or?

So I suppose your patch makes sense to me, although I am not familiar enough to understand the undef case... Perhaps some comment on how these three cases can emerge and may be treated would be good? (at least to me :-)

bjope added a subscriber: bjope.Fri, Jan 24, 2:43 PM
bjope added inline comments.
llvm/lib/CodeGen/BranchFolding.cpp
861

Afaict this isn't consistent with the mirrored situation (if MO says "undef" and OtherMO says "killed") the result will be that "undef" is dropped, but not an addition of "killed".
I think the result should be the same regardless of which bb that use as a base for the merge. Shouldn't it?

Btw, is there a problem with simply clearing the killed flag unless it is set in both operands?
(I'm not sure if an undef implies killed today. Or if it that might change in the future when the freeze stuff has been implemented. In your example above instr2 is a def, but I assume it could be a use as well, that could be undef, or maybe even a use of the same undef value as used in instr1.)