This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add transfer_op LoadToStore forwarding and deadStore optimizations
ClosedPublic

Authored by ThomasRaoux on Nov 11 2020, 9:33 PM.

Details

Summary

Add transformation to be able to forward transfer_write into transfer_read operation and to be able to remove dead transfer_write when a transfer_write is overwritten before being read.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Nov 11 2020, 9:33 PM
ThomasRaoux requested review of this revision.Nov 11 2020, 9:33 PM
ThomasRaoux retitled this revision from [mlir][vector] Add transfer_op LoadToStore and deadStore optimizations to [mlir][vector] Add transfer_op LoadToStore forwarding and deadStore optimizations.Nov 11 2020, 9:33 PM
aartbik added inline comments.Nov 13 2020, 4:31 PM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
272

This analysis transfer ops .... -> I cannot parse this part, is something missing?

mlir/lib/Dialect/Vector/VectorTransferOpTransforms.cpp
29

Returns (capital R, possibly s form)

32

can even be a for-loop without body!

37

empty line after (if we enclose longer regions, I prefer more whitespace)

also for the closing }

57

period at end, otherwise starts new sentence
(or use comma)

87

typo: potentially

91

typoe: overwriting

115

does not need {}

157

typo: dominated

172

to many "it"s
It there is a write, but its memory if disjoint with ... we can ignore the write ....

mlir/test/Dialect/Vector/vector-transferop-opt.mlir
17

can you cleanup the layout of this file for 80-colums, so it reads a bit easier

Address review comments.

ThomasRaoux marked 10 inline comments as done.Nov 16 2020, 11:00 AM
ThomasRaoux added inline comments.
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
272

I removed this sentence as it wasn't clear indeed and didn't add much.

aartbik added inline comments.Nov 19 2020, 10:54 AM
mlir/include/mlir/Dialect/Vector/VectorUtils.h
165

perhaps a bit nitpicky, but I find the isXXX easier to read than the plural areXXX
can't we keep it singular with e.g. isDisjointTransferSet or something like that?

mlir/lib/Dialect/Vector/VectorTransferOpTransforms.cpp
91

found such a ....?

100

can you make this cascade of if statements a if/elseif/elseif sequence
since very branch has a continue, I find that a bit easier to read and get the logic

223

this sentence is missing something

224

typo: opportunity

mlir/lib/Dialect/Vector/VectorUtils.cpp
331

sentence does not flow?

Address review comments

ThomasRaoux marked 4 inline comments as done.Nov 19 2020, 11:37 AM
ThomasRaoux added inline comments.
mlir/lib/Dialect/Vector/VectorTransferOpTransforms.cpp
100

I'm not sure there is a nice way to do that since for the second if not all the cases end with a continue. I replaces the second continue with an else, hopefully that makes it clearer.

mlir/lib/Dialect/Vector/VectorUtils.cpp
331

It was part of the code moved. I fixed the sentence based on what I think the original author meant.

aartbik accepted this revision.Nov 20 2020, 11:23 AM
aartbik added inline comments.
mlir/lib/Dialect/Vector/VectorTransferOpTransforms.cpp
40

last nit (but this is subjective so feel free to ignore). The "OpOpt" is a bit of a mouthful.
You could just call this TransferOptimization.

This revision is now accepted and ready to land.Nov 20 2020, 11:23 AM

Thanks Aart.

mlir/lib/Dialect/Vector/VectorTransferOpTransforms.cpp
40

Done.