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

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

Orlando added a reviewer: debug-info.
Orlando added a comment.

In D150577#4345930 <https://reviews.llvm.org/D150577#4345930>, @bjope wrote:

> In D150577#4345584 <https://reviews.llvm.org/D150577#4345584>, @Orlando wrote:
>
>> Hi @bjope
>>
>> 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).

Aha, yeah that makes sense. Thanks for the explanation!

Since `rfindDebugLoc` wants to get the `DebugLoc` of the passed-in iterator/instruction, as you explained, it feels like maybe it should be the caller's responsibility to filter out `instr_rend()`. However, `findDebugLoc` gracefully handles being handled `instr_end()` (by returning an empty `DebugLoc`), so handling `instr_rend` in `rfindDebugLoc` seems like the least surprising option.

Whether it's better to return an empty `DebugLoc` (because the iterator is invalid) or scan forward for the next one (because `rend` points to somewhere before the start, and `rfindDebugLoc` finds the "this _or next_" filtered `DebugLoc`) I am not entirely sure as I think both make sense. What are your thoughts on this? I am happy to accept this as it is, but I want to add some other debug-info reviewers in case anyone has a stronger opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150577