This is an archive of the discontinued LLVM Phabricator instance.

LiveIntervalAnalysis: fix a crash in repairOldRegInRange
ClosedPublic

Authored by nhaehnle on Aug 9 2016, 3:14 AM.

Details

Summary

I'm not very familiar with this code, so feel free to take over if you
have a better approach to fixing this.

See the new test case for one that was (non-deterministically) crashing
on trunk and deterministically hit the assertion that I added in D23302.
Basically, the machine function contains a sequence

DS_WRITE_B32 %vreg4, %vreg14:sub0, ...
DS_WRITE_B32 %vreg4, %vreg14:sub0, ...
%vreg14:sub1<def> = COPY %vreg14:sub0

and SILoadStoreOptimizer::mergeWrite2Pair merges the two DS_WRITE_B32
instructions into one before calling repairIntervalsInRange.

Now repairIntervalsInRange wants to repair %vreg14, in particular, and
ends up trying to repair %vreg14:sub1 as well, but that only becomes
active _after_ the range that is to be repaired, hence the crash due
to LR.find(...) == LR.begin() at the start of repairOldRegInRange.

I believe that just skipping those subrange is fine, but again, not too
familiar with that code.

Diff Detail

Event Timeline

nhaehnle updated this revision to Diff 67310.Aug 9 2016, 3:14 AM
nhaehnle retitled this revision from to LiveIntervalAnalysis: fix a crash in repairOldRegInRange.
nhaehnle updated this object.
nhaehnle added subscribers: MatzeB, llvm-commits.
kparzysz edited edge metadata.Aug 9 2016, 9:25 AM

This looks ok to me.

lib/CodeGen/LiveIntervalAnalysis.cpp
1409

LaneMask is probably always non-zero.

1411

Remove the "else".

nhaehnle updated this revision to Diff 67365.Aug 9 2016, 10:16 AM
nhaehnle marked 2 inline comments as done.
nhaehnle edited edge metadata.

Rearrange the check.

kparzysz accepted this revision.Aug 9 2016, 10:19 AM
kparzysz edited edge metadata.
This revision is now accepted and ready to land.Aug 9 2016, 10:19 AM
This revision was automatically updated to reflect the committed changes.
MatzeB added inline comments.Dec 5 2016, 4:09 PM
llvm/trunk/lib/CodeGen/LiveIntervalAnalysis.cpp
1399–1403 ↗(On Diff #67571)

I am not sure this is correct.

This seems to hit all cases where the first live segments ends after endIdx, though the comment sounds like it wants to hit the cases where the first live segments begins after endIdx.

MatzeB edited edge metadata.Dec 5 2016, 4:19 PM

Reviewing this because of http://llvm.org/PR31204.

I think this should be:

if (LII != LR.end() && LII->start < endIdx)
  lastUseIdx = LII->end;
else if (LII == LR.begin()) {
  // We may not have a liverange at all if this is a subregister untouched
  // between \p Begin and \p End.
} else
  --LII;

Maybe you can test/update this before it gets pulled into 3.9.x?