This is an archive of the discontinued LLVM Phabricator instance.

[LV] Set name for vector preheader and trip count check blocks
Needs ReviewPublic

Authored by ebrevnov on Dec 10 2019, 3:24 AM.

Details

Summary

Current implementation doesn't set block name where trip count check is generated. As a result it's hard to identify such checks in IR. With this change we will unconditionally set appropriate names for vector preheader and trip count check blocks. That simplifies reading IR.

Event Timeline

ebrevnov created this revision.Dec 10 2019, 3:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 3:24 AM
ebrevnov edited the summary of this revision. (Show Details)Dec 10 2019, 3:33 AM
ebrevnov added reviewers: hsaito, Ayal, fhahn, rengolin, anna.
ebrevnov retitled this revision from [LV] Set and preserve name of vector preheader block. to [LV] Set name for vector preheader and trip count check blocks.
lebedev.ri added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2701–2705

passing-by note: here and everywhere: should it really be just "tc.check"? shouldn't that be a suffix added to the preexisting loop name?

This change may conflict with your other two. How are we supposed to review them? Are they a set?

If so, what's the order in which they have to be applied?

If this is just a naming issue, and the test changes look all related to naming, then I agree with @lebedev.ri.

This should be a suffix to the current name, otherwise you'll end up with multiple %tc.check.N still having to find which branch leads where.

This change may conflict with your other two. How are we supposed to review them? Are they a set?

If so, what's the order in which they have to be applied?

Yes there are a dependencies between change sets. Especially this and next one. The order is defined by parent/child relationship and can be found at "Revision Contents" section Stack tab.

If this is just a naming issue, and the test changes look all related to naming, then I agree with @lebedev.ri.

This should be a suffix to the current name, otherwise you'll end up with multiple %tc.check.N still having to find which branch leads where.

I agree with you guys adding a prefix to existing loop name would produce more descriptive names. Unfortunately, this is not what vectorizer does today. And there was no intention to change that in this patch. I believe change like that will touch LOT of tests.
In fact, this patch just fixes inconsistencies in current naming scheme. Namely it does two things: 1) Introduces "tc.check" name for a block which does trip count checking. 2) Sets 'vector.ph' name for a vector preheader even if existing block is reused.
Note that next patch enables unconditional generation of new vector preheader block thus tc.check & vector.ph blocks will contain only related instructions and nothing else.

ebrevnov updated this revision to Diff 234221.Dec 17 2019, 12:04 AM

Tests update