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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

> haven't gone through the unit test yet

I focused my review on the unittest parts. I reviewed the tests closely up to and including `MLocSimpleLoop`, after which I started skimming a little (see the inline nit/comment about improving test readability).

Once both of our nits have been addressed, assuming @StephenTozer is happy with the code changes he reviewed, I think we can consider both halves of this patch reviewed. So, LGTM.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:55
+/// only the variables and blocks in a single lexical scope, exploiting their
+/// locality.
 ///
----------------
👍 


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:112
+    auto *TRI = MF->getRegInfo().getTargetRegisterInfo();
+    // Slow, but works
+    for (unsigned int I = 1; I < TRI->getNumRegs(); ++I) {
----------------
nit: missing a full stop.


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:119
+
+    // If this ever fails, something is very wrong with this unit test.
+    return 0;
----------------
Can you add an assert / llvm_unreachable?


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:232
+  EXPECT_EQ(OutLocs[0], ValueIDNum(0, 1, RspLoc));
+  EXPECT_EQ(OutLocs[1], ValueIDNum(0, 0, RspLoc)); // Rax contains RspLoc
+}
----------------
nit: full stop.


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


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:376
+  EXPECT_EQ(OutLocs[3][0], LiveInRsp);
+}
+
----------------
👍 


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:427
+
+  // First off: everything though be live-through.
+  initValueArray(InLocsPtr, 3, 2);
----------------
nit: could you fix this comment?


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:480
+  EXPECT_EQ(OutLocs[2][0], LiveInRsp);
+  // rax,
+  EXPECT_EQ(InLocs[0][1], LiveInRax);
----------------
nit: wobbly comment


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:487
+  EXPECT_EQ(OutLocs[2][1], LiveInRsp);
+}
+
----------------
:thumbsup:


================
Comment at: llvm/unittests/CodeGen/InstrRefLDVTest.cpp:548
+  ValueIDNum LiveInRsp(0, 0, RspLoc);
+  ValueIDNum RspPHIInBlk1(1, 0, RspLoc);
+  ValueIDNum RspDefInBlk1(1, 1, RspLoc);
----------------
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);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110173