Page MenuHomePhabricator

[LV][NFC] Keep dominator tree up to date during vectorization.
ClosedPublic

Authored by ebrevnov on Dec 4 2019, 9:50 PM.

Details

Summary

Currently, DT is not consistent during vector code generation. It's partially updated during code generation and there is a big fix up at the end. Moreover DT updates has strong dependence on fixed structure of vector loop. This is bad for at least two reasons. For upcoming change https://reviews.llvm.org/D71053 we should be able to skip some parts of code generation meaning that vector code structure is not fixed any more.

Second, there is an existing TODO in the code for this:

  • // FIXME: After creating the structure of the new loop, the dominator tree is
  • // no longer up-to-date, and it remains that way until we update it
  • // here. An out-of-date dominator tree is problematic for SCEV,
  • // because SCEVExpander uses it to guide code generation. The
  • // vectorizer use SCEVExpanders in several places. Instead, we should
  • // keep the dominator tree up-to-date as we go.

With this change DT is updated as we go and always reflects current CFG. That means transformations dependent on DT should work correctly now. In addition we can easily skip any part of vector code generation without breaking DT consistency.

Event Timeline

ebrevnov created this revision.Dec 4 2019, 9:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 9:50 PM
ebrevnov updated this revision to Diff 232265.Dec 4 2019, 10:52 PM

Minor update.

ebrevnov edited the summary of this revision. (Show Details)Dec 4 2019, 10:58 PM
ebrevnov edited the summary of this revision. (Show Details)
ebrevnov added reviewers: Ayal, hsaito, anna.
ebrevnov edited the summary of this revision. (Show Details)
fhahn requested changes to this revision.Dec 5 2019, 12:37 AM
fhahn added a subscriber: fhahn.

Please split the patch into one that only preserves the DT directly, without any other changes.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2683–2686

Change unrelated to keeping the DT up to date?

2706

Please use SplitBlock from BasicBlockUtils.h and pass the DT instead of updating it manually (here and at other paces in the patch) Same for LoopInfo.

This revision now requires changes to proceed.Dec 5 2019, 12:37 AM
ebrevnov updated this revision to Diff 232303.Dec 5 2019, 3:45 AM

Extracted formatting and renaming to a separate review.

ebrevnov updated this revision to Diff 232304.Dec 5 2019, 3:51 AM

Restore lost change.

ebrevnov marked 2 inline comments as done.Dec 5 2019, 3:55 AM
ebrevnov added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2706

I tried to use it but something was not working for me initially. I'll give it another try since I believe that was simply my mistake.

ebrevnov updated this revision to Diff 232513.Dec 6 2019, 2:04 AM

Use SplitBlock utility to automatically udpate DT and LI.

ebrevnov updated this revision to Diff 232514.Dec 6 2019, 2:09 AM

Minor formatting update.

Harbormaster completed remote builds in B42003: Diff 232514.
ebrevnov updated this revision to Diff 232516.Dec 6 2019, 2:13 AM

Minor naming update

fhahn added a comment.Dec 6 2019, 1:55 PM

IIUC ideally there should be no changes to the tests required to update the DT handling.

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

This is also unrelated to preserving the DT and LI as we go along. Please strip from this patch.

2705

Also unrelated to preserving the DT & LI?

2742

Unrelated?

2794

unrelated?

2964

unrelated?

3127

This needs wrapping in LLVM_DEBUG or using NDEBUG

ebrevnov marked 5 inline comments as done.Dec 9 2019, 2:09 AM

IIUC ideally there should be no changes to the tests required to update the DT handling.

Now, this is NFC change. Everything else goes to a separate review.

Thanks
Evgeniy

ebrevnov updated this revision to Diff 232781.Dec 9 2019, 2:16 AM

Updating as requested by Florian.

ebrevnov retitled this revision from [LV] Keep dominator tree up to date during vectorization. to [LV][NFC] Keep dominator tree up to date during vectorization..Dec 20 2019, 10:22 PM
fhahn accepted this revision.Dec 21 2019, 8:22 AM

LGTM thanks!

Can you please update the patch description before committing.

This revision is now accepted and ready to land.Dec 21 2019, 8:22 AM
xbolva00 added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.cpp
514

#ifdef EXPENSIVE_CHECKS?

ebrevnov edited the summary of this revision. (Show Details)Dec 22 2019, 7:56 PM

LGTM thanks!

Thanks!

Can you please update the patch description before committing.

Done.

ebrevnov marked an inline comment as done.Dec 22 2019, 8:05 PM
ebrevnov added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.cpp
514

Please note, in the original code there is an unconditional verification at the end of 'updateAnalysis'. Since 'updateAnalysis' was removed we need to do verification here to preserve original behavior. I'm not sure if it makes sense to change that...

xbolva00 added inline comments.Dec 25 2019, 4:44 AM
llvm/lib/Transforms/Vectorize/VPlan.cpp
514

Ok, thanks

ebrevnov closed this revision.Dec 30 2019, 4:25 AM

ebrevnov committed rG948e745270de: [LV][NFC] Keep dominator tree up to date during vectorization. (authored by ebrevnov).