[PATCH] D150577: [CodeGen] Fix for MachineBasicBlock::rfindDebugLoc(instr_rend())

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

Orlando added a comment.

Hi @bjope

As I understand it, this change means the behaviour of `rfindDebugLoc` when given `instr_rend()` is now similar to the existing behaviour of `findPrevDebugLoc` when given `instr_end()`. `findPrevDebugLoc` calls `prev_nodbg` (code <https://github.com/llvm/llvm-project/blob/095e6ac9fd92d03dcb1e19b60cb06a8140aae69d/llvm/include/llvm/CodeGen/MachineBasicBlock.h#L1323>) which decrements the iterator before calling `skipDebugInstructionsBackward`. I.e. In both cases an `end` iterator is valid input and will not be dereferenced. (SGTM)

Is there a reason that `findDebugLoc` and `rfindDebugLoc` call `skipDebugInstructionsForward` / `skipDebugInstructionsBackward` rather than `next_nodbg` / `prev_nodbg`. It looks like calling the `nodbg` functions could solve the problem (the end-iterator-dereference) too, with no new code, but I am not certain of that without testing it.



================
Comment at: llvm/unittests/CodeGen/MachineBasicBlockTest.cpp:64
+
+  // Insert two MI with DebugLoc DL1 and DL2..
+  MCInstrDesc MCID = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
----------------
tiny nit: double full stop here


================
Comment at: llvm/unittests/CodeGen/MachineBasicBlockTest.cpp:71
+
+  // Tests with two MI:s.
+  EXPECT_EQ(DL1, MBB.findDebugLoc(MBB.instr_begin()));
----------------
misplaced `:` in comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150577