This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix incorrect removal of COPYs in AArch64RedundantCopyElimination
AbandonedPublic

Authored by yutsumi on Oct 3 2021, 11:02 PM.

Details

Summary

Don't remove COPYs with implicit defs, unless we know the implicitly
defined register is also zero.

Removing the following mov instruction generated from a COPY
is invalid if the upper 32 bits of x1 is non-zero and x1 will be used.
So, we should avoid this removal.

cbnz w1, b2
b1:
mov x1, wzr // set all 64 bits of x1 zero
// code assuming upper half of x1 is non-zero

The same fix https://reviews.llvm.org/D29850 as this was abandoned
probably because the above instruction seemed not to be generated.
But the generation of it is depending on the passes executed
before, e.g., register allocator.
So, this assumption should not be made and we should
avoid removing the above COPYs as well as MOVi32imms.

Event Timeline

yutsumi created this revision.Oct 3 2021, 11:02 PM
yutsumi requested review of this revision.Oct 3 2021, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2021, 11:02 PM
yutsumi edited the summary of this revision. (Show Details)Oct 4 2021, 1:53 AM
yutsumi edited the summary of this revision. (Show Details)
Matt added a subscriber: Matt.Oct 5 2021, 12:50 PM

Do you have a example of this going wrong from a ll file or C example? I can see how this case is incorrect, but I'd like to try and trace through a full example and understand how these things get created.

@dmgreen No. I don't have ll file or C/C++ code now.

What was the original case that was going wrong then?

After my colleague modified llvm/lib/CodeGen/CalcSpillWeights.cpp for the register allocator in the LLVM 7 based compiler, he faced the Segmentation fault when executing a program.
As a result of an investigation, we found that the modification for the register allocator is not bad, and this fix is needed.
The above modification for the register allocator cannot be applied to the current main branch straightly, and cannot be published because that modification is in our product code.

OK. This does alter codegen without changed to the register allocator, and I wanted to look into why.

What would happen if we had this? We should still know that the top bits are 0:

  $w8 = ...
  CBNZW $w8, %bb.2
bb.1:
  $w8 = COPY $wzr, implicit-def $x8
  ...
yutsumi abandoned this revision.Nov 8 2021, 1:29 AM

Sorry for the late reply.
I found that the problem this patch intended to fix was fixed by https://reviews.llvm.org/D87771 .