[PATCH] D98218: [LSR] fix a issue that LoopStrengthReduction drop debug location unnecessarily

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

I think this is good to go, with some revisions to the test.



================
Comment at: llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll:2
+; RUN: opt < %s -loop-reduce -S 2>&1 | FileCheck %s
+; CHECK: icmp uge i32 %{{[a-zA-Z_][a-zA-Z0-9_]*}}.next, %{{[a-zA-Z_][a-zA-Z0-9_]*}}, !dbg !21
+
----------------
The CHECK line should try to closely match the correct conditions for the test to pass in -- you don't need to regex the SSA-Value names as they're very stable, and don't often change in small tests like these.

Instead, you should check that the correct metadata is attached ("!dbg !21"). The metadata numbers can often change when LLVM alters how it uses metadata, so you shouldn't use literal numbers in the CHECK line. Instead, you should capture the number (see the FileCheck documentation, or other debug-info tests) and then check that there's a DILocation metadata node with the same metadata number.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll:3
+; CHECK: icmp uge i32 %{{[a-zA-Z_][a-zA-Z0-9_]*}}.next, %{{[a-zA-Z_][a-zA-Z0-9_]*}}, !dbg !21
+
+; ModuleID = 'simplified-dbg.bc'
----------------
Could you include a top level comment explaining what this is testing for -- it makes it easier for people in the future to understand what the intention of the test was.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/optimizemax_debugloc.ll:33
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
----------------
We usually remove function attributes (unless they're necessary) from debug-info tests, just to reduce future maintenence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98218