[PATCH] D39986: [LSR] Expand: Use the replaced value's debug loc (PR25630)

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4940
+  DebugLoc Loc;
+  if (auto *I = dyn_cast<Instruction>(LF.OperandValToReplace))
+    Loc = I->getDebugLoc();
----------------
aprantl wrote:
> What besides an instruction could this be? An Argument?
I'm not sure, but I haven't been able to prove that this is always an Instruction.


================
Comment at: test/Transforms/LoopStrengthReduce/pr25630.ll:1
+; RUN: opt -loop-reduce < %s -S -o - | FileCheck %s
+
----------------
qcolombet wrote:
> Could you use a more descriptive name for the filename?
> Then, mention the PR in the comment here.
Will fix.


================
Comment at: test/Transforms/LoopStrengthReduce/pr25630.ll:57
+  %spec.select.2 = select i1 %cmp1.2, i32 30, i32 %add.i.2, !dbg !32
+  tail call void @llvm.dbg.value(metadata i32 %inc.1, metadata !24, metadata !DIExpression()), !dbg !25
+  tail call void @llvm.dbg.value(metadata i32 %spec.select.2, metadata !23, metadata !DIExpression()), !dbg !26
----------------
aprantl wrote:
> it might make sense to remove all the dbg.value intrinsics that are not used by the test and remove all the metadata that depends on them to make the test easier to read/maintain
Will fix.


================
Comment at: test/Transforms/LoopStrengthReduce/pr25630.ll:89
+
+attributes #0 = { nounwind readnone ssp uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="penryn" "target-features"="+cx16,+fxsr,+mmx,+sse,+sse2,+sse3,+sse4.1,+ssse3,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
----------------
aprantl wrote:
> we usually strip out all non-essential attributes from tests
Will fix.


https://reviews.llvm.org/D39986