In preparation for allowing InstrRefBasedLDV to handle DBG_VALUE_LIST, this patch updates the internal representation that it uses to represent debug values to store a list of values. This is one of the more significant changes in terms of line count, but is fairly simple and should not affect the output of this pass.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM with some minor comments. I'd feel more confident if someone else (/ @jmorse ) took a quick look too, but given this is NFC and conceptually straightforward, I'm happy to approve it.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
219 | has this been clang-formated (IIRC we don't usually indent for the namespace scope)? | |
282 | It looks like you use this union for type punning. IIRC this is UB in C++ but I am unsure. Is this a case that is permitted for some reason, do you know? | |
373 | IIRC there is at least one other place in the compiler where there's a limit for number of debug operands (SCEV or salvaging or both?). Have you considered using a single value for all the limits? Or, if not, that then maybe updating the other limits if they're greater than 8. |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
282 | I don't know actually - but this file was already using this type of behaviour beforehand in ValueIDNum. All the same, it shouldn't be difficult to do this without type punning - I'd be interest in getting a solid answer on whether it should be outright banned or not (I think in C++ it's still a relatively common pattern?). | |
373 | I agree, having a global limit would be mostly good - it would also be good to have some good benchmarks for testing different expression lengths, so that we can have an objective indication of what's optimal. |
Update unit tests. The changes are all trivial; the changes were performed primarily by regex substitution, and simply update DbgValue constructors in the tests to use a DbgOpID instead of a ValueIDNum, creating the former in most cases with a test function addValueDbgOp that creates an ID for a Value in the standard way. For the vlocJoin tests, there was no need to create the underlying value, as vlocJoin does not actually need to view it - it only checks equality and const-ness, which can be done entirely from the ID.
llvm/unittests/CodeGen/InstrRefLDVTest.cpp | ||
---|---|---|
1811 | this patch seems to have added a lot of unused variables that some bots are complaining about, e.g. https://buildkite.com/llvm-project/upstream-bazel/builds/37703#0182c6bb-7631-491a-8b71-3ed8ae1ef7d6 could you take a look? |
llvm/unittests/CodeGen/InstrRefLDVTest.cpp | ||
---|---|---|
1811 | I just landed 58ad7555f564fbcd63d8d782f4fb7f6f2d432fe9. |
llvm/unittests/CodeGen/InstrRefLDVTest.cpp | ||
---|---|---|
1811 | Thanks for catching that, was just reverting - I'll take a look at the unused variable warnings now. |
llvm/unittests/CodeGen/InstrRefLDVTest.cpp | ||
---|---|---|
1811 | I see you've caught those as well - sorry about that! |
has this been clang-formated (IIRC we don't usually indent for the namespace scope)?