[PATCH] D110173: [DebugInfo][InstrRef] Use correct (and existing) PHI placement utilities for machine locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

NB: I've also fiddled with the mloc tests so that they clear and rebuild the transfer function between each call to buildMLocValueMap, that way you don't have to keep "what's in the transfer function" in mind throughout.



================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:119
+
+    // If this ever fails, something is very wrong with this unit test.
+    return 0;
----------------
Orlando wrote:
> Can you add an assert / llvm_unreachable?
I think this is probably discouraged in unit tests, as it blocks all the other CodeGen tests from running; on the other hand, the test configuration is completely broken if it fires, so SGTM


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:288
+  ValueIDNum RspPHIInBlk3(3, 0, RspLoc);
+  ValueIDNum LiveInRax(0, 0, RaxLoc);
+  ValueIDNum RaxLiveInBlk1(1, 0, RaxLoc);
----------------
Orlando wrote:
> `LiveInRax` is unused in this test. Is that intentional?
Ah, yeah, this is a copy+paste error


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:548
+  ValueIDNum LiveInRsp(0, 0, RspLoc);
+  ValueIDNum RspPHIInBlk1(1, 0, RspLoc);
+  ValueIDNum RspDefInBlk1(1, 1, RspLoc);
----------------
Orlando wrote:
> I think that the tests, especially this one, would be easier to read if there was a comment mapping the block numbers to the block names used in the CFG comment at the start of each test. Or, slightly more radically, replace the integer literals with a name. E.g.
> ```
> const uint64_t Entry = 0, Loop1 = ...;
> LiveInRsp(Entry, 0, RspLoc);
> ```
> 
Probably yes; although then it probably wouldn't all nicely align with fixed-width fonts then :(

I'll try to do this more in future tests, would rather not rewrite this one though.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110173/new/

https://reviews.llvm.org/D110173