This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fix crash when scheduling DBG_VALUE
ClosedPublic

Authored by arsenm on Nov 15 2017, 4:47 PM.

Details

Reviewers
vpykhtin
rampitec
Summary

This calls handleMove with a DBG_VALUE instruction,
which isn't tracked by LiveIntervals. I'm not sure
this is the correct place to fix this. The generic
scheduler seems to have more deliberate region
selection that skips dbg_value.

The test is also really hard to reduce. I haven't been able
to figure out what exactly causes this particular case to
try moving the dbg_value.

Diff Detail

Event Timeline

arsenm created this revision.Nov 15 2017, 4:47 PM
rampitec added inline comments.Nov 15 2017, 4:55 PM
lib/Target/AMDGPU/GCNSchedStrategy.cpp
334–335

I am afraid this will push RegionBegin iterator. Maybe you need to skip a debug value in the loop at line 389 instead.

arsenm added inline comments.Nov 16 2017, 3:49 PM
lib/Target/AMDGPU/GCNSchedStrategy.cpp
334–335

That's what I did originally, but then it seems to just be adding unneeded instructions to the vector. The loop over this uses MI->getIterator(), so I don't think there's a difference

rampitec added inline comments.Nov 16 2017, 3:54 PM
lib/Target/AMDGPU/GCNSchedStrategy.cpp
334–335

Check statement "RegionBegin = Unsched.front()->getIterator();" below at line 415.

arsenm added inline comments.Nov 17 2017, 4:43 PM
lib/Target/AMDGPU/GCNSchedStrategy.cpp
334–335

I think the only thing that will change is if a dbg_value is considered the start of the region. I'm not sure how exactly dbg_value is supposed to interact with the region. Looking at the generic code it seems like dbg_value isn't considered part of a scheduling region, and is skipped over so maybe this shouldn't be allowing dbg_value as the RegionBegin?

rampitec added inline comments.Nov 17 2017, 4:52 PM
lib/Target/AMDGPU/GCNSchedStrategy.cpp
334–335

It probably should not even come to the scheduling, but since it is here we shall preserve region iterators, otherwise weird errors occur.

arsenm updated this revision to Diff 125401.Dec 4 2017, 12:19 PM

Move check (although I'm still suspicious of this)

rampitec accepted this revision.Dec 4 2017, 1:44 PM

LGTM

This revision is now accepted and ready to land.Dec 4 2017, 1:44 PM
arsenm closed this revision.Dec 4 2017, 7:10 PM

r319732