This is an archive of the discontinued LLVM Phabricator instance.

[SLPVectorizer] Add propagateIRFlagsWithOp() function to propagate IRFlags for specific Operation
ClosedPublic

Authored by dtemirbulatov on Jul 11 2017, 10:50 PM.

Details

Summary

This change is part of https://reviews.llvm.org/D28907.

This is a new functionality that allows to propagate IR flags only for specific operation OpValue parameter.

Diff Detail

Event Timeline

dtemirbulatov created this revision.Jul 11 2017, 10:50 PM
RKSimon edited edge metadata.

A few minor comments

lib/Transforms/Vectorize/SLPVectorizer.cpp
225 ↗(On Diff #106140)

Put the Flag set: on the last line to make it clearer.

228 ↗(On Diff #106140)

Should we assume this (the cast will assert on failure) or check for it?

if (auto *Intersection = dyn_cast<Instruction>(OpValue)) {
}
4585 ↗(On Diff #106140)

Should the last argument be I[0]?

filcab requested changes to this revision.Jul 12 2017, 4:05 AM

I haven't reviewed the usages of propagateIRFlagsWithOp, but this really needs test cases. Please add them.

lib/Transforms/Vectorize/SLPVectorizer.cpp
222 ↗(On Diff #106140)

Does this need doxygen comments, or would a // be enough?
Please reformat the comment. The line widths aren't consistent.

227 ↗(On Diff #106140)

Remove indentation levels like you do further in the function:

auto *VecOp = dyn_cast<Instruction>(I);
if (!VecOp)
  return;
// rest of the function
This revision now requires changes to proceed.Jul 12 2017, 4:05 AM

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.
I'm not sure if Z having those flags is on purpose, though. And I don't see what we get from that.

Please tell me if I misunderstood something. I'm not the most familiar with the SLP vectorizer.

lib/Transforms/Vectorize/SLPVectorizer.cpp
226 ↗(On Diff #106140)

Alternative name: propagateIRFlagsToOp since you're propagating them to that specific op. What do you think?

2858 ↗(On Diff #106140)

What will be the difference between the old code and this one?

dtemirbulatov added inline comments.Jul 12 2017, 8:33 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
222 ↗(On Diff #106140)

There was already doxygen comments before for this function, why to remove those?
oh, thanks, I will reformat.

225 ↗(On Diff #106140)

ok

226 ↗(On Diff #106140)

looks good

227 ↗(On Diff #106140)

ok, correct

228 ↗(On Diff #106140)

ok

2858 ↗(On Diff #106140)

Here we propagate flags to V0 if only EvenScalars[0].getCode() equals to operations in V0 with similar Code, Also note how we gather flags in the new version and set those flags after that.

4585 ↗(On Diff #106140)

correct.

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.

propagateIRFlagsWithOp only collects flags from Y if Y.Code == Z.Code while propagateIRFlags copies any flag from Y.

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.

propagateIRFlagsWithOp only collects flags from Y if Y.Code == Z.Code while propagateIRFlags copies any flag from Y.

Please can you explain that difference from propagateIRFlags in the comment?

I'm not sure if Z having those flags is on purpose, though. And I don't see what we get from that.

Please tell me if I misunderstood something. I'm not the most familiar with the SLP vectorizer.

Usually it should have flags in common and it collects those flags see "Intersection->andIRFlags(V);" in the loop across all VL,
but I have one example from test/Transforms/SLPVectorizer/X86/horizontal-list.ll where NUW flag was canceled by NSW.
VL[0]: %r1 = add nuw i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

Resulting in to:
VL[0]: %r1 = add i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

dtemirbulatov added inline comments.Jul 13 2017, 12:54 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
227 ↗(On Diff #106140)

ok

dtemirbulatov edited edge metadata.

Fixed issue with test/Transforms/SLPVectorizer/X86/horizontal-list.ll, Merged two versions of propagateIRFlags into one.

RKSimon added inline comments.Jul 14 2017, 1:43 AM
lib/Transforms/Utils/LoopUtils.cpp
1383

These must be dyn_cast and we must check for null.

1398

clang-format this whole function.

update after rksimon's remarks.

filcab accepted this revision.Jul 18 2017, 3:22 PM

LGTM

include/llvm/Transforms/Utils/LoopUtils.h
535

Maybe "If OpValue is non-null, we only consider operations similar to OpValue when intersecting"?

lib/Transforms/Utils/LoopUtils.cpp
1396

Merge these:

if (OpValue == nullptr || Opcode == Instr->getOpcode())
  VecOp->andIRFlags(V);
This revision is now accepted and ready to land.Jul 18 2017, 3:22 PM
This revision was automatically updated to reflect the committed changes.