loop-reduce pass seems to drop debug location unnecessarily

Quuxplusone commented 4 years ago


Bugzilla Link	PR48067
Status	CONFIRMED
Importance	P enhancement
Reported by	Yuanbo Li (yuanboli233@gmail.com)
Reported on	2020-11-03 20:23:38 -0800
Last modified on	2020-12-01 14:33:00 -0800
Version	trunk
Hardware	PC Linux
CC	aprantl@apple.com, jeremy.morse.llvm@gmail.com, josh@joshmatthews.net, llvm-bugs@lists.llvm.org, vk@vedantk.com
Fixed by commit(s)
Attachments	`abc.ll` (22668 bytes, text/plain) `file_48067.txt` (2997 bytes, text/plain)
Blocks	PR31268
Blocked by
See also

Created attachment 24130
the IR file that triggers the potential bug

Overview: In some cases, it seems that the loop-reduce pass will drop debug
location unnecessarily.

Steps to reproduce:

Here is my version of llvm:

$ clang++ --version
clang version 12.0.0 (https://github.com/llvm/llvm-project.git
f847094c246810d698d3d6d476f066e9b8779456)
Target: x86_64-unknown-linux-gnu

The IR file abc.ll is uploaded in the attachment. It is a file obtained from
LLVM regression tests (llvm/test/Transforms/LoopStrengthReduce/X86/2008-08-14-
ShadowIV.ll). We run debugify pass to generate artificial debugging information
for the IR file to get the current version of abc.ll.

$ /home/absozero/trunk/root-clang/bin/opt -loop-reduce abc.ll > result.bc
$ /home/absozero/trunk/root-clang/bin/llvm-dis result.bc

Then we can get result.ll as the output of the loop-reduce pass.

$ diff abc.ll result.ll | head -n 30
(omit some irrelevant text)......
26,28c25,27
<   %exitcond = icmp eq i32 %indvar.next, %umax, !dbg !27
<   call void @llvm.dbg.value(metadata i1 %exitcond, metadata !17, metadata
!DIExpression()), !dbg !27
<   br i1 %exitcond, label %return, label %bb, !dbg !28
---
>   call void @llvm.dbg.value(metadata i1 %scmp, metadata !17, metadata
!DIExpression()), !dbg !27
>   %scmp = icmp uge i32 %indvar.next, %n
>   br i1 %scmp, label %return.loopexit, label %bb, !dbg !28
(omit some irrelevant text)......

After the loop-reduce, the variable %exitcond is replaced by another variable
%scmp. However, the variable %scmp seems to drop the debug location !dbg !27
unnecessarily.

Quuxplusone commented 4 years ago

Attached abc.ll (22668 bytes, text/plain): the IR file that triggers the potential bug

Quuxplusone commented 4 years ago

This optimization seems to be related to optimizeMax function in the
LoopStrengthReduce.cpp file. Specifically, the replacement of %exitcond to
%scmp should be related to the following lines in the source code:

  // Ok, everything looks ok to change the condition into an SLT or SGE and
  // delete the max calculation.
  ICmpInst *NewCond =
    new ICmpInst(Cond, Pred, Cond->getOperand(0), NewRHS, "scmp");

  // Delete the max calculation instructions.
  Cond->replaceAllUsesWith(NewCond);

If the debug location is not intentionally dropped, then a potential patch may
be inserting "NewCond->setDebugLoc(Cond->getDebugLoc());" before the
replaceAllUsesWith function call.

Quuxplusone commented 4 years ago

Yup, this looks faulty, certainly in the reproducer.

Yuanbo wrote:
> If the debug location is not intentionally dropped, then a potential patch may
> be inserting "NewCond->setDebugLoc(Cond->getDebugLoc());" before the
> replaceAllUsesWith function call.

Using the principles in:

  https://llvm.org/docs/HowToUpdateDebugInfo.html

There's a one-to-one relationship between the new and old instruction, and it's
placed in the same location, so the DebugLoc should be preserved in the
reproducer.

I think the general case is a bit cloudier though: the new compare instruction
is moved around by some logic in OptimizeLoopTermCond that I don't immediately
understand. That might cause it to cross a block boundary, in which case we
might need to merge / drop DebugLocs at that point.

Quuxplusone commented 3 years ago

Attached file_48067.txt (2997 bytes, text/plain): the reduced IR simplified.ll

Quuxplusone / LLVMBugzillaTest

loop-reduce pass seems to drop debug location unnecessarily #47036