Page MenuHomePhabricator

[Loop Vectorize] Added a separate metadata
ClosedPublic

Authored by DIVYA on Aug 2 2017, 7:05 AM.

Details

Summary

Added a separate metadata to indicate when the loop has already been vectorized instead of setting width and count to 1.

Worked in collaboration with Aditya Kumar

Diff Detail

Repository
rL LLVM

Event Timeline

DIVYA created this revision.Aug 2 2017, 7:05 AM
hfinkel edited edge metadata.Aug 2 2017, 7:11 AM

Added a separate metadata to indicate when the loop has already been vectorized instead of setting width and count to 1.

What's the motivation?

lib/Transforms/Vectorize/LoopVectorize.cpp
1320 ↗(On Diff #109340)

These messages are read by users so referring to internal variables is not appropriate. How about, "or the loop has already been vectorized".

fhahn added a subscriber: fhahn.Aug 2 2017, 7:34 AM
DIVYA updated this revision to Diff 109997.Aug 7 2017, 8:01 AM
  • modified comments
DIVYA marked an inline comment as done.Aug 7 2017, 8:01 AM
bjope added a subscriber: bjope.Aug 10 2017, 5:15 AM
hfinkel added inline comments.Aug 11 2017, 9:09 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
1285 ↗(On Diff #109997)

space after if.

1285 ↗(On Diff #109997)

I think we should add a comment here explaining this. How about?

// If the vectorization width and interleaving count are both 1 then consider the loop to have been already vectorized because there's nothing more that we can do.
1286 ↗(On Diff #109997)

Which needs to be:

Width.Value == 1 && Interleave.Value == 1

As you have it written, the condition will be true whenever there are any overlapping set bits in the two numbers, say when they're both equal, which is not what we want.

DIVYA updated this revision to Diff 110794.Aug 11 2017, 1:04 PM
  • Added Comment
DIVYA marked 3 inline comments as done.Aug 11 2017, 1:06 PM
hfinkel accepted this revision.Aug 11 2017, 1:25 PM

LGTM

lib/Transforms/Vectorize/LoopVectorize.cpp
1287 ↗(On Diff #110794)

Please make sure this line is not more than 80 cols.

This revision is now accepted and ready to land.Aug 11 2017, 1:25 PM
DIVYA updated this revision to Diff 110812.EditedAug 11 2017, 2:17 PM
  • modified comment formatting
DIVYA marked an inline comment as done.Aug 11 2017, 2:18 PM
DIVYA added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
1287 ↗(On Diff #110794)

Done

This revision was automatically updated to reflect the committed changes.

I have a question:

Is there any reason not to mark the scalar loop and the vectorized version with the "llvm.loop.vectorize.width" and "llvm.loop.interleave.count" metadata reflecting the real outcome from the LV.
I.e 1, 1 for the scalar loop and VF, UF for the vectorized version? That would be possible after this change.

Our out of tree compiler have later loop passes that could benefit from that information. I do not know if would be of any use for any of the in-tree targets or if it would hurt something.

I have a question:

Is there any reason not to mark the scalar loop and the vectorized version with the "llvm.loop.vectorize.width" and "llvm.loop.interleave.count" metadata reflecting the real outcome from the LV.
I.e 1, 1 for the scalar loop and VF, UF for the vectorized version? That would be possible after this change.

Our out of tree compiler have later loop passes that could benefit from that information. I do not know if would be of any use for any of the in-tree targets or if it would hurt something.

I'm not clear on how a later pass might use that information in a robust way. Can you please elaborate?