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

Authored by vsk on Mon, Nov 13, 3:17 PM.

Details

Summary

Fix PR25630 by setting the debug location of code generated in
LSRInstance::Expand() to the location attached to the replaced value.

Depends on D39985

Diff Detail

vsk created this revision.Mon, Nov 13, 3:17 PM
qcolombet accepted this revision.Mon, Nov 13, 4:39 PM

LGTM. Nitpicks below.

test/Transforms/LoopStrengthReduce/pr25630.ll
1 ↗(On Diff #122738)

Could you use a more descriptive name for the filename?
Then, mention the PR in the comment here.

This revision is now accepted and ready to land.Mon, Nov 13, 4:39 PM
aprantl added inline comments.
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4949

What besides an instruction could this be? An Argument?

test/Transforms/LoopStrengthReduce/pr25630.ll
57 ↗(On Diff #122738)

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

89 ↗(On Diff #122738)

we usually strip out all non-essential attributes from tests

vsk updated this revision to Diff 122770.Mon, Nov 13, 8:45 PM
vsk marked 3 inline comments as done.
vsk retitled this revision from WIP: [LSR] Expand: Use the replaced value's debug loc (PR25630) to [LSR] Expand: Use the replaced value's debug loc (PR25630).
vsk edited the summary of this revision. (Show Details)
  • Address review feedback from Quentin and Adrian.
  • Add test coverage for the logic in GenerateIVChain, and for more users of LSRInstance::Expand.
vsk added inline comments.Mon, Nov 13, 8:47 PM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4949

I'm not sure, but I haven't been able to prove that this is always an Instruction.

test/Transforms/LoopStrengthReduce/pr25630.ll
1 ↗(On Diff #122738)

Will fix.

57 ↗(On Diff #122738)

Will fix.

89 ↗(On Diff #122738)

Will fix.

aprantl accepted this revision.Tue, Nov 14, 7:03 AM
aprantl added inline comments.
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4949

Might as well use a cast<> then, it will assert and we'll find out :-)
Hm.. it could be a constant, perhaps?

vsk added inline comments.Tue, Nov 14, 11:20 AM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4949

If we use a cast<>, test/Transforms/LoopStrengthReduce/illegal-addr-modes.ll fails because the operand val is an i8*.

aprantl added inline comments.Tue, Nov 14, 11:23 AM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4949

thanks for checking!

davide accepted this revision.Tue, Nov 14, 11:26 AM

This one LGTM, thanks Vedant!