[PATCH] D123679: [RISCV] Don't getDebugLoc for the end node of MBB iterator

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Drive by review from a debug-info perspective -- I'm not a RISC-V person so don't feel confident enough to approve this, but I can see how the removed code will crash in the presence of a block only containing debug instructions, and that the new code will not crash. The fix looks correct in that sense, but someone else should confirm that it's not going to change codegen.

On the test front, IMO this is much better written as a MIR test that only runs prologepilog, where it will be resistant to optimisations, and the input to prologepilog will be very clear. It will also allow you to test that the correct (possibly empty) DebugLoc is assigned to instructions by the relevant function.



================
Comment at: llvm/test/CodeGen/RISCV/xform_ah-min.ll:2
+; RUN: llc -mtriple=riscv64 -enable-shrink-wrap < %s
+; Check that we don't crash
+
----------------
It's also best practice to check at least one line of output to ensure that what you want to happen, actually happened. That ensures the test doesn't spuriously pass in the future. An example would be if the IR Verifier was tightened to reject the debug-info in this file, it would load and compile this IR and pass the test, but the scenario you're testing wouldn't be covered.

Adding a CHECK line, for example stopping after prologepilog and checking the relevant block has the right contents, would prevent that.


================
Comment at: llvm/test/CodeGen/RISCV/xform_ah-min.ll:41-42
+
+attributes #0 = { argmemonly mustprogress nofree norecurse nosync nounwind readonly willreturn "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+64bit,+a,+c,+m,+relax,-save-restore" }
+attributes #1 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
----------------
Could you remove any unnecessary attributes, potentially all of them -- they're a future maintenance burden we can avoid now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123679