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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D150577#4345584 <https://reviews.llvm.org/D150577#4345584>, @Orlando wrote:

> 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)

Right. This patch is making the end iterators valid input. Basically treating `end()`/`rend()` as pointing to an extra MI with an undefined DebugLoc after/before the last/first MI.

> 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.

The  `findDebugLoc` and `rfindDebugLoc` functions are taking the DebugLoc from the MI pointed to by the iterator, if that MI fulfil the requirements. So they kind of do "skip-debug + examine MI + step-iterator", while the findPrev variants do "step-iterator + skip-debug + examine MI". The `next_nodbg` / `prev_nodbg` is a short form for doing "step + skip", so in order to use them one would need to rewrite the code a bit more.

It had perhaps been more interesting to have helpers for scanning forward/backward, and doing pre/post increment. That would give four different variants. Now we got two variants that scan forward doing post-increment. And two variants that scan backwards doing pre-increment. I got a bit confused about that for awhile (and it is not that easy to understand what prev/next, first/last, forward/backward etc means when having variants using both iterators and reverse iterators). One idea with the unittests I added is to actuallly show what happens when using those helpers (even if one still would need to add some debug instructions to make those tests even more complete).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150577