This is an archive of the discontinued LLVM Phabricator instance.

[debuginfo] Vectorizing a loop doesn't terminate all vectorized variable locations
AbandonedPublic

Authored by chrisjackson on Oct 29 2020, 12:51 PM.

Details

Summary

This is an initial fix for BZ39018. The source file loop induction variable and its debug users and set to undef to prevent misleading values being presented. This is the simple 'don't mislead the user' fix, but there are possible improvements.

I intend to expand this work so that rather than permanently setting the original induction variable to 'undef', its value will reflect the the value for the vectorzied loops. For example, if a single iteration of the vectorized loop has width 4, compared to the previous 1, then the induction variable should increase in value by '4' per iteration.

There is some additional complexity as the vectorizer may use vector instructions of varying width to 'fit' the maximum iterations defined in the original source.

Diff Detail

Event Timeline

chrisjackson created this revision.Oct 29 2020, 12:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 12:51 PM
chrisjackson requested review of this revision.Oct 29 2020, 12:51 PM
fhahn added a subscriber: fhahn.Nov 26 2020, 5:55 AM
fhahn added inline comments.
llvm/test/DebugInfo/pr39018.ll
1 ↗(On Diff #301712)

the test is for functionality in LoopVectorize. Should we also have a test in test/Transforms/LoopVectorize? I think we have other debug related tests there.

44 ↗(On Diff #301712)

The test seems much more verbose than necessary to test the patch. For example, instead of the memset & allocas, you could just pass noalias pointers. Also, are 2 loops needed? From the patch, I'd assume no.

fhahn added inline comments.Nov 26 2020, 6:06 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3701

nit: no llvm:: prefix required.

3709

Looks like some spaces are missing here. Could you run clang-format on the diff?

Addressed review comments:

  • Simplified lit test and changed location
  • Applied clang format
djtodoro added inline comments.
llvm/test/Transforms/LoopVectorize/pr39018.ll
33

We are usually able to remove llvm.lifetime.start/end intrinsics from the debug info related tests.

71–74

I guess we don't need these attributes.

109

I think we can remove these !tbaa metadata.

chrisjackson updated this revision to Diff 309996.EditedDec 7 2020, 12:34 PM

Removed excess intrinsics, attributes and metadata from test.

I think you accidentally deleted your patch :-)

Can replaceDbgUsesWithUndef from Local.cpp be used here?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3716

dependent

Restored erroneously removed source patch!

Simplified setting to undef using replaceDbgUsesWithUndef().

Looking at the output of the test, it looks like the dbg.value(undef) is in the scalar tail of the loop, rather than vector.body, right? This is slightly cross purposes to what we want, I think: the induction variable value is unavailable during the vectorized portion of the loop and should be undef, but the scalar part isn't optimised, and so the value is still available.

IMO: it'd be sufficient to collect debug users of the induction variable and add new undefs at the entry to the vectorized loop.

(I'm basing this on e6eaacbf0bd , sorry if my observations are because LoopVectorize has changed a lot over the months I've been ignoring your patch ._. )

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3705–3706

Could I suggest switching "variable" to "debug variable" here -- it makes it clearer that we're dealing with debug-info instead of some functional portion of the vectorised loop.

llvm/test/Transforms/LoopVectorize/pr39018.ll
2–3

Nit, "updates" is a bit like a commit message saying "updates": could you elaborate on what's supposed to happen, i.e.:

  • The loop gets vectorised,
  • Earlier variable locations should be terminated with an undef.

It'll help future readers work out what's going on.

ormris removed a subscriber: ormris.Jun 3 2021, 10:53 AM
chrisjackson abandoned this revision.Feb 22 2022, 1:28 AM