This is an archive of the discontinued LLVM Phabricator instance.

[mlir][SPIRVToLLVM] Add shufflevector conversion for static index
ClosedPublic

Authored by Weiwei-2021 on Oct 20 2021, 10:46 AM.

Details

Summary

Add the shufflevector conversion. It only handles the static, i.e., IntegerAttr, index.

I am not sure whether it is ok to create multiple constantOps with the same value in llvm dialect, therefore I add a utility to find existing constant Op for a specific value. Correct me if we don't need to do it.

Co-authored: Xinyi Liu <xyliuhelen@gmail.com>

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Oct 20 2021, 10:46 AM
Weiwei-2021 requested review of this revision.Oct 20 2021, 10:46 AM
antiagainst requested changes to this revision.Oct 26 2021, 1:57 PM
antiagainst added inline comments.
mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
1376

Can use OpAdaptor instead of ArrayRef<Value> so that you can access vector1() and vector2() using methods.

1380

You should use vector1 and vector2 from operands. Dialect conversion works by going through each op in a block sequentially and covert them one by one. The ops generating vector1 and vector2 might be converted previously (and with the converted vector1 and vector2 stored in operands) so we need to use the converted version for them.

1387

Can return early here. We typically prefer early return in MLIR.

1390

This does not need to be converted again?

1393

This map should not be needed. You can create duplicated constants; that's quite common in MLIR.

1396

For unsupported cases, we should fail the pattern match. Assertion should not be used here.

This revision now requires changes to proceed.Oct 26 2021, 1:57 PM
Weiwei-2021 marked 6 inline comments as done.
Weiwei-2021 added inline comments.
mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
1376

Cool. Do not know that the OpAdaptor can be used in this way. Thanks.

1393

Got it. Thanks.

antiagainst accepted this revision.Oct 29 2021, 4:14 PM

Nice, just two small issues. Feel free to land after fixing them!

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
1388

Given we have an early return in the above, we can remove the { .. } for the else and save one level of indent. :) That's the typical coding style in MLIR.

1411

Please use the input op's location. Unknown location will make print/debug hard.

This revision is now accepted and ready to land.Oct 29 2021, 4:14 PM
Weiwei-2021 marked 2 inline comments as done.Nov 1 2021, 7:43 AM

Thank you for your comment. Fixed and will land soon!

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
1411

good to know it. I see other places are using unknownLoc, thought unknown is expected for new Ops like constant.

This revision was automatically updated to reflect the committed changes.
Weiwei-2021 marked an inline comment as done.