[PATCH] D77576: [globalisel] Add lost debug locations verifier

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 5 inline comments as done.
dsanders added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LostDebugLocObserver.h:36-39
+  void createdInstr(MachineInstr &MI) override;
+  void erasingInstr(MachineInstr &MI) override;
+  void changingInstr(MachineInstr &MI) override;
+  void changedInstr(MachineInstr &MI) override;
----------------
arsenm wrote:
> Defined to no-op calls in debug build?
I assume you mean the non-debug build.

I suppose we could do as there's a small code-size saving from it. It shouldn't affect execution time though as we just decline to install the observer for that case


================
Comment at: llvm/lib/CodeGen/GlobalISel/Legalizer.cpp:369
+                                      /*MBB=*/&*MF.begin());
+    R << "lost " << std::to_string(LocObserver.getNumLostDebugLocs())
+      << " debug locations during pass";
----------------
paquette wrote:
> paquette wrote:
> > Can this use NV?
> > 
> > E.g. something like
> > 
> > ```
> > R << "lost " << NV("NumLostDebugLocs", LocObserver.getNumLostDebugLocs()) << " debug locations during pass";
> > ```
> > 
> > I think that would make optimization records easier to parse, if you choose to save them.
> > 
> > It would also be nice to have a testcase for the remarks.
> Although that being said, any testcase that exposes this is a bug we should fix, so... maybe it's not reasonable to test it. :)
> 
> It would be nice to know what the opt remark output looks like though.
> Can this use NV?

I didn't know about NV. Fixed in my working copy

> It would also be nice to have a testcase for the remarks.

A test case is tricky because as you say, we should fix all the cases where this actually happens.

> It would be nice to know what the opt remark output looks like though.

I'll add an example in a comment


================
Comment at: llvm/lib/CodeGen/GlobalISel/Legalizer.cpp:370
+    R << "lost " << std::to_string(LocObserver.getNumLostDebugLocs())
+      << " debug locations during pass";
+    reportGISelFailure(MF, TPC, MORE, R);
----------------
arsenm wrote:
> Missing newline?
As far as I know, it's not needed for remarks. The other two above don't have a newline at least


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-fdiv.mir:3
 # RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=legalizer  %s -o - | FileCheck -check-prefix=SI  %s
+# RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=mir-debugify,legalizer -mir-debug-loc=0  %s -o - | FileCheck -check-prefix=SI  %s
 # RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=fiji -run-pass=legalizer  %s -o - | FileCheck -check-prefix=VI  %s
----------------
vsk wrote:
> arsenm wrote:
> > Do we really need to double the number of run lines in every test?
> There are some alternatives:
> - Get rid of the non-mir debugify check lines. This is fine by me, but some pass authors might not like this.
> - Run the tests with `llvm-lit -Dopt='opt -debugify` -Dllc='llc -run-pass=mir-debugify'`. That means only a subset of users/bots are test with debug info.
> Do we really need to double the number of run lines in every test?

I kept it to the ones that actually reveal something but I agree it's not great to have the extra run lines. Not least because they don't actually do anything different to the main ones on non-asserts builds because the LostDebugLocObserver doesn't get installed. For that build, they're just wasting time.

I want to end up at a -verify-mir-debug-locations similar to -verify-machineinstrs (however that spelling is a bit misleading because each pass must cooperate to verify anything) or `-mir-debugify-before-all -mir-strip-debug-after-all` or something along those lines. At that point injecting the options via lit makes a lot of sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77576