Page MenuHomePhabricator

Extended LoadStoreVectorizer to vectorize subchains.
ClosedPublic

Authored by asbirlea on Jul 7 2016, 4:20 PM.

Details

Summary

LSV used to abort vectorizing a chain for interleaved load/store accesses that alias.
Allow a valid prefix of the chain to be vectorized, mark just the prefix and retry vectorizing the remaining chain.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea updated this revision to Diff 63158.Jul 7 2016, 4:20 PM
asbirlea retitled this revision from to Extended LoadStoreVectorizer to vectorize subchains..
asbirlea updated this object.
asbirlea added reviewers: llvm-commits, jlebar, arsenm.
jlebar added inline comments.Jul 7 2016, 6:42 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
113 ↗(On Diff #63158)

Please update comment.

114 ↗(On Diff #63158)

This probably also deserves a new name.

453 ↗(On Diff #63158)

Suppose we have

  • load
  • load
  • fn_with_side_effects
  • load

Don't we want to (try to) vectorize the first two loads? Or, do you want to handle that in a separate patch?

460 ↗(On Diff #63158)

Nit, I'm not wild about reusing "Idx" for something different. (I mean, it's still an index, but over a completely different list.)

675 ↗(On Diff #63158)

Calling this param "FullChain" is weird, since when we call this recursively, we pass what I can only call a partial chain. :)

701 ↗(On Diff #63158)

Is "VectorizedSet" the right name? In this case we're saying that we didn't vectorize anything (and we're returning false), yet we're adding these instructions to the set...

704 ↗(On Diff #63158)

If this is just describing the call to getBoundaryInstrs, I don't think it needs a comment. If it's describing something else, can we reword it? I don't think "enclosing" is the right word. Maybe "Get the first and last instructions in FullChain," but that's basically exactly what getBoundaryInstrs' comment says.

707 ↗(On Diff #63158)

Nit, end sentences with periods.

714 ↗(On Diff #63158)

This indentation is very confusing. I think it would be OK to put the comment inside the "else"? In which case I'd suggest also putting braces (although some folks around here would leave them out, which really mystifies me).

716 ↗(On Diff #63158)

VectorizedSet->push_back(FullChain.front());

718 ↗(On Diff #63158)

I think this would be clearer as two separate if statements (repeating the "return false"). I'd rather repeat one short line than add an extra level of nesting.

if (StopChain == 0) {
  // No vectorization possible, bail out.
  VectorizedSet->append(...);
  return false;
}
if (StopChain == 1) {
  // Failed after the first instruction.  Discard it and try the smaller chain.
  VectorizedSet->push_back(FullChain.front());
  return false;
}
761 ↗(On Diff #63158)

"discarded for future vectorization" is awkward. Maybe "We won't try again to vectorize the elements of chain, regardless of whether we succeed below."

asbirlea updated this revision to Diff 63359.Jul 8 2016, 5:13 PM

Address comments.

jlebar added inline comments.Jul 8 2016, 10:55 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
443 ↗(On Diff #63359)

Did you intend to address this comment in your latest update?

asbirlea added inline comments.Jul 11 2016, 7:31 AM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
443 ↗(On Diff #63359)

No, let's handle that in a separate patch. Yes, it's another feature that should be added.

jlebar added inline comments.Jul 11 2016, 10:45 AM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
112 ↗(On Diff #63359)

The first sentence in this comment no longer really applies. Can we make this more cohesive?

113 ↗(On Diff #63359)

Per IRL, maybe vectorizablePrefixEnd or vectorizablePrefixEndIdx would be a better name?

357 ↗(On Diff #63359)

Hm, why is this diff showing up here? Shouldn't this be part of D22071?

450 ↗(On Diff #63359)

Well, calling the one "Idx" and the other "Index" doesn't entirely solve the problem of using the same name for the two variables. Now they're different names to the computer, but still basically the same name to humans. :)

How about InstrIdx and ChainIdx? Then you could rename VIdx to InstrIdx, which would be a plus for readability, I think (since these *are* the same indices). (The problem with s/Idx/VIdx/ is that the first loop is really over instructions, not Values.)

624 ↗(On Diff #63359)

Capital letters for variables. Although, why did we lose the range-based for loop in the first place? Was it just so we could do Head = -1? If so, a flag variable would be much more clear, I think.

658 ↗(On Diff #63359)

We shouldn't need a separate variable for this. Can we just pass VectorizedValues (needs to be renamed) to vectorizeLoadChain directly (after changing the type accepted by that function)?

asbirlea updated this revision to Diff 63605.Jul 11 2016, 3:59 PM

Update to latest.

asbirlea updated this revision to Diff 63745.Jul 12 2016, 4:01 PM

Address comments.

asbirlea marked 6 inline comments as done.Jul 12 2016, 4:08 PM
asbirlea added inline comments.
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
112 ↗(On Diff #63605)

Well, it still applies. It's still checking for the existance of those instructions but using the info differently. I shortened the comment somewhat.

356 ↗(On Diff #63605)

Because I accidentally did the updates on this one some time ago :). Then I redid them on the other one.

449–455 ↗(On Diff #63605)

Also updated VV/V and VVIdx/VIdx. They still need different names from InstrIndx, since the same index is used for both Chain and Mem in the loop above, and now we need two.

623–632 ↗(On Diff #63605)

For Heads it's fine to use the range-based loop (updated). For Tails I need the iterator to get Heads[TailsIterator].

jlebar accepted this revision.Jul 12 2016, 4:34 PM
jlebar edited edge metadata.
jlebar added inline comments.
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
112 ↗(On Diff #63605)

I like it now. :)

Except we should be consistent *at least within a single comment* as far as whether we're using indicative ("checks") or imperative ("return").

623–632 ↗(On Diff #63605)

OK. I would still very much prefer to use a separate flag variable rather than repurposing Head for that purpose, though.

435 ↗(On Diff #63745)

Do we need to declare E? Can we just say I != To?

465 ↗(On Diff #63745)

Please be sure to clang-format the lines you've touched in this patch (e.g. with git-clang-format HEAD^) before submitting.

This revision is now accepted and ready to land.Jul 12 2016, 4:34 PM
asbirlea updated this revision to Diff 63755.Jul 12 2016, 4:44 PM
asbirlea marked 4 inline comments as done.
asbirlea edited edge metadata.

Address comments.

This revision was automatically updated to reflect the committed changes.