[PATCH] D78151: [AArch64InstrInfo] Ignore debug insts in canInstrSubstituteCmpInstr [5/10]

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1450
     const MachineInstr &Instr = *I;
+    if (Instr.isDebugInstr())
+      continue;
----------------
vsk wrote:
> paquette wrote:
> > fhahn wrote:
> > > It seems like a few similar changes popped up recently. It might be worth making it a bit easier to iterate over a range of MIs while skipping debug instructions automatically. It's quite easy to construct iterator ranges that skip debug instructions (see snippet below). It might be worth adding a helpers like that. They could also naturally be used for the existing skipDebugInstructionsBackward/skipDebugInstructionsForward
> > > 
> > > ```
> > > +  auto MIsWithoutDbg =
> > > +      make_filter_range(make_range(std::next(CmpInstr->getIterator()),
> > > +                                   CmpInstr->getParent()->instr_end()),
> > > +                        [](MachineInstr &I) { return !I.isDebugInstr(); });
> > > +  for (const MachineInstr &Instr : MIsWithoutDbg) {
> > > ```
> > +1, it seems like this is an extremely easy mistake to make, and it's probably peppered around basically everywhere.
> Thanks for bringing this up. It's not always feasible to use the range-loop idiom, but I agree that it's a good fit here. In cases where the loop iterator is either initialized before the for-init, or modified inside the for-body -- maybe not.
> 
> I experimented with some different approaches. Adding 'nodbg' filter_iterator variants for the various MachineBasicBlock iterators did not seem good to me, as it's quite intrusive: a lot of code has to change to adopt the new iterators. Adding helpers that replace std::{next, prev} (but skip debug instructions) could work, but adopting these turned out to be messier than I would've liked. In the end I chose to adapt what we have at the IR-level, but to allow for existing iterators to be passed in.
> 
> Zooming way out, I think the long-term plan should be to remove debug instructions from the instruction stream entirely. The recent callSiteInfo work proves this can work. A prerequisite step might be to have DBG_VALUE point to a MachineInstr, like @jmorse proposed in http://lists.llvm.org/pipermail/llvm-dev/2020-February/139440.html.
> I experimented with some different approaches. Adding 'nodbg' filter_iterator variants for the various MachineBasicBlock iterators did not seem good to me, as it's quite intrusive: a lot of code has to change to adopt the new iterators. Adding helpers that replace std::{next, prev} (but skip debug instructions) could work, but adopting these turned out to be messier than I would've liked. In the end I chose to adapt what we have at the IR-level, but to allow for existing iterators to be passed in.

That seems reasonable to me. Having custom start/end points for iterators seems much more common in the backend than in the middle end,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78151