This is an archive of the discontinued LLVM Phabricator instance.

Fix possible assertion when using PBQP with debug info
ClosedPublic

Authored by danilaml on Mar 13 2020, 5:56 AM.

Details

Summary

Skip debug instructions before calling functions not expecting them.
In particular, LIS.getInstructionIndex(*mi) would fail if mi was a debg instr.

Diff Detail

Event Timeline

danilaml created this revision.Mar 13 2020, 5:56 AM
danilaml edited the summary of this revision. (Show Details)Mar 13 2020, 6:07 AM
danilaml updated this revision to Diff 250194.Mar 13 2020, 7:11 AM
danilaml edited the summary of this revision. (Show Details)
danilaml marked an inline comment as done.

Test generated from the following C code:

#include <stdint.h>

void test(int16_t* a, int16_t* b, int16_t* c, uint32_t n) {
    for (int32_t i = 0; i < n; i++) {
        *c++ = *a++ + *b++;
    }
}
llvm/lib/CodeGen/CalcSpillWeights.cpp
218

Don't think that counting uses in debug instrs was originally intended for this heuristic.

Looks generally good, except for the test.

llvm/test/CodeGen/Generic/csw-debug-assert.ll
2

Please add a comment explaining what is being tested here

Please add something that explicitly CHECKs the results? Otherwise this test will succeed even if someone symlinks /bin/true to llc :-)

54

Please delete all nonnecessary attributes, it makes the tests hard to maintain

danilaml updated this revision to Diff 250233.Mar 13 2020, 9:31 AM
danilaml marked 2 inline comments as done.
danilaml added inline comments.Mar 13 2020, 9:31 AM
llvm/test/CodeGen/Generic/csw-debug-assert.ll
2

Done.

From what it seems, the majority of the lit tests are vulnerable to /bin/true trick ;P

54

Done. It's a bit hard to know what's necessary or not when ll is generated from simple C code and not hand-crafted (and you can't just remove all metadata).

danilaml updated this revision to Diff 250240.Mar 13 2020, 9:44 AM
danilaml edited the summary of this revision. (Show Details)
aprantl added inline comments.Mar 13 2020, 9:45 AM
llvm/test/CodeGen/Generic/csw-debug-assert.ll
12

How about checking at least some of the contents of the DBG_VALUE?

110

You can probably significantly shorten the test by manually assigning all instructions the same !dbg location.

aprantl accepted this revision.Mar 13 2020, 9:46 AM

LGTM with all outstanding comments addressed.

This revision is now accepted and ready to land.Mar 13 2020, 9:46 AM
danilaml updated this revision to Diff 250566.Mar 16 2020, 8:27 AM
danilaml marked 2 inline comments as done.
danilaml marked an inline comment as done.Mar 16 2020, 8:28 AM
danilaml added inline comments.
llvm/test/CodeGen/Generic/csw-debug-assert.ll
110

I've simplified the test. Do you think manual dbg hacking is still necessary?

danilaml marked an inline comment as done.Mar 16 2020, 8:31 AM
aprantl added inline comments.Mar 16 2020, 9:59 AM
llvm/test/CodeGen/Generic/csw-debug-assert.ll
77

Do you need the TBAAA info?

110

Getting the tests as minimal as possible helps in the long term with maintenance. All tests need to updated occasionally for various IR changes and then it really helps to know what is relevant to the test and what isn't. So, I would do it, since it's not a lot of work and helps focus on what's important.

danilaml marked an inline comment as done.Mar 16 2020, 10:25 AM
danilaml added inline comments.
llvm/test/CodeGen/Generic/csw-debug-assert.ll
110

My concern here that by manually editing .ll metadata I might accidentally break some underlying assumption which might lead to problems later down the line. Anyway, I'll try to clean DILocations up.

danilaml updated this revision to Diff 250786.Mar 17 2020, 8:16 AM
danilaml marked 2 inline comments as done.Mar 17 2020, 8:33 AM
This revision was automatically updated to reflect the committed changes.