This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Improve crash on invalid mapping
ClosedPublic

Authored by arsenm on Dec 14 2018, 12:24 AM.

Details

Reviewers
bogner
Summary

If NumBreakDowns is 0, BreakDown is null.
This trades a null dereference with an assert somewhere
else.

Diff Detail

Event Timeline

arsenm created this revision.Dec 14 2018, 12:24 AM
bogner added a subscriber: bogner.Dec 15 2018, 11:33 AM

I don’t think I understand what this is trying to accomplish. ISTM that this will now hit a null dereference on line 123 when we read BreadDown in the case that NumBreakDowns is zero, but this is to avoid hitting an assert somewhere else? If this is to make it clearer as to what’s failing, shouldn’t we assert NumBreakDowns != 0 here instead of just hoping to crash on a null dereference?

I don’t think I understand what this is trying to accomplish. ISTM that this will now hit a null dereference on line 123 when we read BreadDown in the case that NumBreakDowns is zero, but this is to avoid hitting an assert somewhere else? If this is to make it clearer as to what’s failing, shouldn’t we assert NumBreakDowns != 0 here instead of just hoping to crash on a null dereference?

No, that is what this avoids. Right now there is a null dereference on line 123. Now this function exits, so this continues until it hits
Assertion failed: (getOperandMapping(Idx).isValid() && "We must have a mapping for reg operands"), function verify, file ../lib/CodeGen/GlobalISel/RegisterBankInfo.cpp, line 572.

bogner accepted this revision.Dec 17 2018, 3:19 PM

I don’t think I understand what this is trying to accomplish. ISTM that this will now hit a null dereference on line 123 when we read BreadDown in the case that NumBreakDowns is zero, but this is to avoid hitting an assert somewhere else? If this is to make it clearer as to what’s failing, shouldn’t we assert NumBreakDowns != 0 here instead of just hoping to crash on a null dereference?

No, that is what this avoids. Right now there is a null dereference on line 123. Now this function exits, so this continues until it hits
Assertion failed: (getOperandMapping(Idx).isValid() && "We must have a mapping for reg operands"), function verify, file ../lib/CodeGen/GlobalISel/RegisterBankInfo.cpp, line 572.

Sorry, I must've read this diff backwards or something. LGTM.

This revision is now accepted and ready to land.Dec 17 2018, 3:19 PM
arsenm closed this revision.Dec 18 2018, 1:30 AM

r349464