Implemented a skeleton for SPIR-V to LLVM dialect conversion, added conversion for spv.BitwiseAndOp

[georgemitenkov]

Summary

These commits set up the skeleton for SPIR-V to LLVM dialect conversion.
I created SPIR-V to LLVM pass, registered it in Passes.td, InitAllPasses.h.
Added a pattern for spv.BitwiseAndOp and tests for it. Integer, float and vector types are converted through LLVMTypeConverter.

[rriddle]

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

16

These headers don't look necessary. Can you replace with forward declarations instead?

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.h

16

This doesn't seem necessary, can you use forward declarations instead?

mlir/lib/Conversion/CMakeLists.txt

18

Please keep this sorted.

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

30

using namespace mlir;

42

This should already be guaranteed, please avoid asserting on things that are already guaranteed by the infra.

45

Remove all trivial braces.

53

This should be mlir::populateSPIRVToLLVMConversionPatterns

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.cpp

44

Remove trivial braces.

[georgemitenkov]

• Refactored code: removed trivial braces, added forward declarations
• Refactored the code with git-clang

Code refactoring

• Removed trivial braces
• Added forward declarations instead of includes
• Made cmake sorted

[ftynse]

Somehow, I now only see the changes made in response to River's comments. Could you please make sure you upload the full patch (arc diff HEAD^ will only include the last commit on your branch for upload in case you have multiple commits; use parent's hash or as many ^^^ as there are commits)

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

1

Nit: pad to 80 cols

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

1

Nit: please pad to 80 cols

[georgemitenkov]

Added 2 previously missing commits

[ftynse]

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

21–23

These two declarations look unused

mlir/test/Conversion/SPIRVToLLVM/convert-to-llvm.mlir

4 ↗

(On Diff #268414)

Please don't pattern-match on SSA value names, https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices

[georgemitenkov]

• Sorted passes in Passes.td and increased padding to 80 cols

Refactored the padding in header files to 80 cols, also put SPIRVToLLVM pass in Pass.td in sorted order with other passes

[georgemitenkov]

• Added correct padding to .cpp files

Added padding of 80 cols to ConvertSPIRVToLLVMPass.cpp and ConvertSPIRVToLLVM.cpp

[antiagainst]

Awesome, George! Nice to see this coming up so fast. Overall looks good to me; I just have a few sytle/comments/etc. nits.

mlir/include/mlir/Conversion/Passes.td

217 ↗

(On Diff #268418)

"Convert SPIR-V dialect to LLVM dialect" to be consistent?

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

1

Convert SPIR-V to LLVM ...

9

SPIR-V dialect to LLVM dialect

25

Populates ...

Typically we prefer to use descriptive instead of imperative words for documentation. (Although this is somehow not very consistent in the codebase...)

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.h

9

from SPIR-V dialect to ...

We are doing more than ops; also types.

23

Creates

mlir/include/mlir/Conversion/VectorToLLVM/ConvertVectorToLLVM.h

16–17

Why this change? Seems unrelated to this patch.

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

26

We'd prefer to not pull in all symbols both from LLVM and spirv together... There are likely ops with the same name and it can cause suprising symbol resolution issue later. Let's just qualify all symbols with LLVM:: or spirv:: properly. :)

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.cpp

38

Please document why we are pulling in std to llvm conversion patterns here. :)

mlir/test/Conversion/SPIRVToLLVM/convert-to-llvm.mlir

3 ↗

(On Diff #268418)

Typically we prefer smaller and dedicated tests. The benefits of doing that is you can isolate culprit lines very easily and updating tests is also easier given you'll not need to update unrelated stuff. So here I'd suggest we create two functions, scalar_bitwise_add and vector_bitwise_and and keep the test cases separate. :)

[antiagainst]

Anther thing about commit message:

• Typically we want a short one-liner to describe what the change is about concisely and put detailed description in a second paragraph separated by an empy line from the one-liner. The benefits of doing so is that it renders nicely on GitHub history UI and also it's very easy to read when using git log --oneline.
• MLIR related changes are typically qualified with [MLIR] at the beginning of the commit message. And then we might also want to further qualify it with [SPIRVToLLVM].

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Changed description style and split tests

Changed description of files to be consistent with the codebase. Added a comment on the use of std to llvm conversion in patterns. Refactored test into 2 tests that test scalar and vector bitwise and separetely. Removed pattern-match on SSA-values.

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Removed unused declarations

Removed unused declarations in ConvertSPIRVToLLVM.h.

[antiagainst]

Please bear with me with more nitpicking. :)

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.cpp

31

This should be put before populateStdToLLVMConversionPatterns so it's near to what's explaining. :)

mlir/test/Conversion/SPIRVToLLVM/convert-to-llvm.mlir

4 ↗

(On Diff #268518)

There is no need to capture the value with T0 here given it's not referenced later. You can just do %{{.*}} here.

10 ↗

(On Diff #268518)

%[[A:.*]] actually captures the matched string into A. Here we are essentially capturing both operands as A (so the later is overwriting the former). Given that we are not particularly interested in the operand here, it's fine to have {{.*}} here.

[antiagainst]

mlir/test/Conversion/SPIRVToLLVM/convert-to-llvm.mlir

10 ↗

(On Diff #268518)

See here for more details: https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-string-substitution-blocks

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Changed test to use string substitution blocks

Put comment about populateStandardToLLVMPatterns next to the actuall function call. Changed FileCheck tests to use substitution blocks {{.*}} instead of saving the matched pattern in [[A:.*]] for example.

[antiagainst]

Awesome, thanks!

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

 //===- ConvertSPIRVToLLVM.h - Convert to LLVM dialect -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Provides patterns to convert SPIR-V ops to LLVM dialect.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVM_H
 #define MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVM_H
 
-#include "mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h"
 #include "mlir/Transforms/DialectConversion.h"
 
 namespace mlir {
+class LLVMTypeConverter;
 class MLIRContext;
 class ModuleOp;
 template <typename T>
 class OperationPass;
 
 /// Populate the given list with patterns that convert from SPIR-V to LLVM.
 void populateSPIRVToLLVMConversionPatterns(MLIRContext *context,
                                            LLVMTypeConverter &typeConverter,
                                            OwningRewritePatternList &patterns);
 
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVM_H

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.h

 //===- ConvertSPIRVToLLVMPass.h - SPIR-V ops to LLVM pass -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Provides a pass to lower from SPIR-V ops to LLVM dialect.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVMPASS_H
 #define MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVMPASS_H
 
-#include "mlir/Pass/Pass.h"
+#include <memory>
 
 namespace mlir {
+class ModuleOp;
+template <typename T>
+class OperationPass;
 
 /// Create a pass to convert SPIR-V operations to the LLVMIR dialect.
 std::unique_ptr<OperationPass<ModuleOp>> createConvertSPIRVToLLVMPass();
 
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_SPIRVTOLLVM_CONVERTSPIRVTOLLVMPASS_H_

mlir/include/mlir/Conversion/VectorToLLVM/ConvertVectorToLLVM.h

 //===- ConvertVectorToLLVM.h - Utils to convert from the vector dialect ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef MLIR_CONVERSION_VECTORTOLLVM_CONVERTVECTORTOLLVM_H_
 #define MLIR_CONVERSION_VECTORTOLLVM_CONVERTVECTORTOLLVM_H_
 
 #include "mlir/Transforms/DialectConversion.h"
 
 namespace mlir {
 class LLVMTypeConverter;
 class ModuleOp;
-template <typename T> class OperationPass;
+template <typename T>
+class OperationPass;
 
 /// Collect a set of patterns to convert from Vector contractions to LLVM Matrix
 /// Intrinsics. To lower to assembly, the LLVM flag -lower-matrix-intrinsics
 /// will be needed when invoking LLVM.
 void populateVectorToLLVMMatrixConversionPatterns(
     LLVMTypeConverter &converter, OwningRewritePatternList &patterns);
 
 /// Collect a set of patterns to convert from the Vector dialect to LLVM.

mlir/lib/Conversion/CMakeLists.txt

 add_subdirectory(AffineToStandard)
 add_subdirectory(AVX512ToLLVM)
 add_subdirectory(GPUCommon)
 add_subdirectory(GPUToNVVM)
 add_subdirectory(GPUToROCDL)
 add_subdirectory(GPUToSPIRV)
 add_subdirectory(GPUToVulkan)
 add_subdirectory(LinalgToLLVM)
 add_subdirectory(LinalgToSPIRV)
 add_subdirectory(LinalgToStandard)
 add_subdirectory(SCFToGPU)
 add_subdirectory(SCFToStandard)
+add_subdirectory(SPIRVToLLVM)
 add_subdirectory(StandardToLLVM)
 add_subdirectory(StandardToSPIRV)
 add_subdirectory(VectorToLLVM)
 add_subdirectory(VectorToSCF)
-add_subdirectory(SPIRVToLLVM)

mlir/lib/Conversion/SPIRVToLLVM/CMakeLists.txt

add_mlir_conversion_library(MLIRSPIRVToLLVM
   intrinsics_gen
 
   LINK_LIBS PUBLIC
   MLIRSPIRV
   MLIRLLVMIR
   MLIRStandardToLLVM
   MLIRIR
   MLIRTransforms
-  )
+  )
 

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

 //===- ConvertSPIRVToLLVM.cpp - SPIR-V to LLVM dialect conversion--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements patterns to convert SPIR-V ops to LLVM ops.
 //
 //===----------------------------------------------------------------------===//
 
+#include "mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h"
 #include "mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h"
 #include "mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/Dialect/SPIRV/SPIRVDialect.h"
 #include "mlir/Dialect/SPIRV/SPIRVOps.h"
 #include "mlir/Dialect/StandardOps/IR/Ops.h"
 #include "mlir/IR/Module.h"
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/Support/LogicalResult.h"
 #include "mlir/Transforms/DialectConversion.h"
 
-#include <iostream>
-
 using namespace mlir;
 using namespace mlir::LLVM;
 using namespace mlir::spirv;
 
-namespace mlir {
+namespace {
 
 class BitwiseAndOpConversion : public ConvertToLLVMPattern {
 public:
   explicit BitwiseAndOpConversion(MLIRContext *context,
                                   LLVMTypeConverter &typeConverter)
       : ConvertToLLVMPattern(BitwiseAndOp::getOperationName(), context,
                              typeConverter) {}
 
   LogicalResult
   matchAndRewrite(Operation *op, ArrayRef<Value> operands,
                   ConversionPatternRewriter &rewriter) const override {
-    assert(operands.size() == 2);
     auto bitwiseAndOp = cast<BitwiseAndOp>(op);
     auto dstType = typeConverter.convertType(bitwiseAndOp.getType());
-    if (!dstType) {
+    if (!dstType)
       return failure();
-    }
     rewriter.replaceOpWithNewOp<LLVM::AndOp>(bitwiseAndOp, dstType, operands);
     return success();
   }
 };
+} // namespace
 
-void populateSPIRVToLLVMConversionPatterns(MLIRContext *context,
-                                           LLVMTypeConverter &typeConverter,
-                                           OwningRewritePatternList &patterns) {
+void mlir::populateSPIRVToLLVMConversionPatterns(
+    MLIRContext *context, LLVMTypeConverter &typeConverter,
   patterns.insert<BitwiseAndOpConversion>(context, typeConverter);
 }
-} // namespace mlir

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.cpp

 namespace {
 /// A pass converting MLIR SPIR-V operations into LLVM dialect.
 class ConvertSPIRVToLLVMPass
     : public ConvertSPIRVToLLVMBase<ConvertSPIRVToLLVMPass> {
   void runOnOperation() override;
 };
 } // namespace
 
 void ConvertSPIRVToLLVMPass::runOnOperation() {
   MLIRContext *context = &getContext();
   ModuleOp module = getOperation();
   LLVMTypeConverter converter(&getContext());
 
   OwningRewritePatternList patterns;
   populateSPIRVToLLVMConversionPatterns(context, converter, patterns);
   populateStdToLLVMConversionPatterns(converter, patterns);
 
   ConversionTarget target(getContext());
   target.addIllegalDialect<spirv::SPIRVDialect>();
   target.addLegalDialect<LLVM::LLVMDialect>();
 
-  if (failed(applyPartialConversion(module, target, patterns, &converter))) {
+  if (failed(applyPartialConversion(module, target, patterns, &converter)))
     signalPassFailure();
-  }
-}
 
 std::unique_ptr<OperationPass<ModuleOp>> mlir::createConvertSPIRVToLLVMPass() {
   return std::make_unique<ConvertSPIRVToLLVMPass>();
 }