This is an archive of the discontinued LLVM Phabricator instance.

[mlir][NFC] Move conversion of scf to spir-v ops in their own file
ClosedPublic

Authored by ThomasRaoux on Jun 30 2020, 2:42 PM.

Details

Summary

Move patterns for scf to spir-v ops in their own file/folder.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Jun 30 2020, 2:42 PM
rriddle added inline comments.Jul 1 2020, 1:47 PM
mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
27

Could you please punt this to a revision that will actually use it?

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
41

Please fix the naming convention here.

63

nit: Move all of these methods out of the anonymous namespace, i.e. just keep the classes.

ThomasRaoux marked an inline comment as done.
ThomasRaoux marked 2 inline comments as done.Jul 1 2020, 2:53 PM
ThomasRaoux added inline comments.
mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
27

I am trying to avoid changing the signature of the populate function twice since those break users (i.e. iree). This is why I added the skeleton for this along with the refactoring. Maybe there is a better flow? If not I would prefer to keep it that way.
BTW, I have a patch up for review using this already (https://reviews.llvm.org/D82246)

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
63

Done. I didn't realized this was the preferred convention. I got bite by a GCC warning in my last patch about defining template specialization outside the anonymous namespace so I thought to be safe I would keep function definitions in there as well. Anyway thanks for letting me know.

rriddle added inline comments.Jul 1 2020, 3:24 PM
mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
27

In cases like this I would much rather prefer that you add code when it is actually going to be used. This addition makes this commit non-NFC because you are adding new functionality, albeit unused.

As for breaking users your internal users(like IREE), you could just wait until all of your dependent revisions are approved before submitting.

antiagainst accepted this revision.Jul 1 2020, 3:42 PM

Thanks for taking care of this! These patterns are hidden in the wrong directory for quite some time so this makes the structure better reflecting their purpose. Thanks! Please move the new functionality to the second patch though.

mlir/include/mlir/Conversion/SCFToSPIRV/SCFToSPIRV.h
2

This needs to be updated.

27

+1. I think we can wait both are approved and land them together and put note for internal integration to pull them together. The title containing "NFC" can be misleading.

35

Collects

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
2

This needs to be updated.

This revision is now accepted and ready to land.Jul 1 2020, 3:42 PM
rriddle added inline comments.Jul 1 2020, 3:55 PM
mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
63

That is a bug in older GCC, and only necessary if the class is templated.

ThomasRaoux edited the summary of this revision. (Show Details)
ThomasRaoux marked 6 inline comments as done.
This revision was automatically updated to reflect the committed changes.