Page MenuHomePhabricator

[SCEV] Apply a single debug loc when expanding a SCEV
Needs ReviewPublic

Authored by vsk on Nov 13 2017, 3:16 PM.



The SCEV expander currently applies debug locations to generated code
based on which location is set at the insertion point. This isn't always
the correct location to apply (see PR25630).

Following the suggestion in the bug report, this patch solves the
problem by introducing an overload of expandCodeFor() which accepts a
DebugLoc. This single location is applied to all instructions generated
while expanding a SCEV.

The existing expand...() methods are updated to use a special location
which conveys that line table information for the expanded code is
unknown. If the function has debug information attached, this special
location is <file>:0:0. Otherwise, the location is empty. The end result
should be conservatively correct.

The existing expand...() methods are also deprecated. My plan is to
audit calls to the deprecated methods and replace them, one-by-one, by
calls which set an appropriate DebugLoc.

Testing: check-llvm

Depends on D39982

Diff Detail

Event Timeline

aprantl added inline comments.

I think DebugLoc is already a value type, so there is no point in passing it by reference.

aprantl added inline comments.Nov 13 2017, 6:56 PM

is this used anywhere?
the coding standard wants functions to begin with lower-case letters.

vsk updated this revision to Diff 122771.Nov 13 2017, 8:48 PM
vsk marked an inline comment as done.
vsk edited the summary of this revision. (Show Details)
  • Pass DebugLoc by value, and add a comment explaining what the PrintTo test helper does.

Will fix.


Yes, it's used by gtest. I'll add a comment.

aprantl added inline comments.Nov 14 2017, 7:06 AM

shouldn't this be \deprecated with a backslash? and come before the declaration?

vsk added inline comments.Nov 14 2017, 11:22 AM

Ouch, I forgot the backslash. I'm not sure what you mean by 'come before the declaration' though - it looks like it's before the declaration already.

aprantl added inline comments.Nov 14 2017, 11:25 AM

it does indeed, sorry.