This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Fix infinite loop in legalization artifact combiner
ClosedPublic

Authored by Petar.Avramovic on Jul 27 2021, 8:10 AM.

Details

Summary

ArtifactValueFinder keeps trying to combine g_unmerge_values in some cases.
Fix is to skip combine attempt for dead defs.

Diff Detail

Event Timeline

Petar.Avramovic requested review of this revision.Jul 27 2021, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 8:10 AM

My best guess is that this happens when dead-def can be found by looking through insert but there is nothing to replace with the value we found.
TODO: avoid using G_INSERT.

arsenm added inline comments.Jul 27 2021, 8:53 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
770

Probably should be use_nodbg_empty

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.ll
22

Should also add a MIR test. Also should add a case where there is only a debug info user

aemerson added inline comments.Jul 27 2021, 12:24 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
770

If the def is dead, we should also set the dead bit in DeadDefs.

Mark dead bit in DeadDefs and add mir test. MIR in last two tests is from just before artifact combiner. In one we can see dead def in unmerge, other has only dbg use.

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir
115–116

With use_nodbg_empty, this will look like DBG_VALUE %dbg_use(s32) because we skip combine (replace register with found value). It also seems wrong to mark register as dead in that case. Should we treat DBG instrs in special way in this combine?

aemerson added inline comments.Jul 28 2021, 5:04 PM
llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir
115–116

If we don't ignore the debug user, then we could have potentially differing codegen between release and debug builds.

aemerson accepted this revision.Jul 29 2021, 9:59 AM
This revision is now accepted and ready to land.Jul 29 2021, 9:59 AM

By the way, I just refactored some of this code in f984b0e177f8 but it's just a case of moving your change to a different function.

This revision was landed with ongoing or failed builds.Aug 2 2021, 3:58 AM
This revision was automatically updated to reflect the committed changes.