Addressed Comments

1. Added the 'final' keyword to ClampFOpConversion
2. Removed string variable; directly add the message string to notifyMatchFailure.
3. Added chipset argument to populateMathToROCDLConversionPatterns instead.

Signed-off-by: Keshav Vinayak Jha <[email protected]>

Author: keshavvinayak01

mlir/include/mlir/Conversion/MathToROCDL/MathToROCDL.h

@@ -9,6 +9,7 @@
 #define MLIR_CONVERSION_MATHTOROCDL_MATHTOROCDL_H_
 
 #include "mlir/Conversion/LLVMCommon/TypeConverter.h"
+#include "mlir/Dialect/AMDGPU/Utils/Chipset.h"
 #include "mlir/IR/PatternMatch.h"
 #include <memory>
 
@@ -20,7 +21,8 @@ class Pass;
 
 /// Populate the given list with patterns that convert from Math to ROCDL calls.
 void populateMathToROCDLConversionPatterns(const LLVMTypeConverter &converter,
-                                           RewritePatternSet &patterns);
+                                           RewritePatternSet &patterns,
+                                           amdgpu::Chipset chipset);
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_MATHTOROCDL_MATHTOROCDL_H_

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

@@ -484,5 +484,5 @@ void mlir::populateGpuToROCDLConversionPatterns(
                GPUSubgroupBroadcastOpToROCDL>(converter);
   patterns.add<GPUSubgroupSizeOpToROCDL>(converter, chipset);
 
-  populateMathToROCDLConversionPatterns(converter, patterns);
+  populateMathToROCDLConversionPatterns(converter, patterns, chipset);
 }

mlir/lib/Conversion/MathToROCDL/MathToROCDL.cpp

@@ -43,8 +43,39 @@ static void populateOpPatterns(const LLVMTypeConverter &converter,
                                            f32ApproxFunc, f16Func);
 }
 
+struct ClampFOpConversion final
+    : public ConvertOpToLLVMPattern<math::ClampFOp> {
+  using ConvertOpToLLVMPattern::ConvertOpToLLVMPattern;
+  ClampFOpConversion(const LLVMTypeConverter &converter,
+                     amdgpu::Chipset chipset)
+      : ConvertOpToLLVMPattern<math::ClampFOp>(converter), chipset(chipset) {}
+
+  LogicalResult
+  matchAndRewrite(math::ClampFOp op, OpAdaptor adaptor,
+                  ConversionPatternRewriter &rewriter) const override {
+    // V_MED3_F16/F32 only exists in gfx9+ artchitectures
+    if (chipset.majorVersion < 9) {
+      return rewriter.notifyMatchFailure(
+          op, ("pre-gfx9 (gfx" + std::to_string(chipset.majorVersion) +
+               "): V_MED_F16 / V_MED3_F32 not supported."));
+    }
+    rewriter.replaceOpWithNewOp<ROCDL::FMed3Op>(op, op.getType(), op.getValue(),
+                                                op.getMin(), op.getMax());
+    return success();
+  }
+  amdgpu::Chipset chipset;
+};
+
+static void addChipsetDependentPatterns(const LLVMTypeConverter &converter,
+                                        RewritePatternSet &patterns,
+                                        amdgpu::Chipset chipset) {
+
+  patterns.add<ClampFOpConversion>(converter, chipset);
+}
+
 void mlir::populateMathToROCDLConversionPatterns(
-    const LLVMTypeConverter &converter, RewritePatternSet &patterns) {
+    const LLVMTypeConverter &converter, RewritePatternSet &patterns,
+    amdgpu::Chipset chipset) {
   // Handled by mathToLLVM: math::AbsIOp
   // Handled by mathToLLVM: math::AbsFOp
   // Handled by mathToLLVM: math::CopySignOp
@@ -119,30 +150,9 @@ void mlir::populateMathToROCDLConversionPatterns(
   // worth creating a separate pass for it.
   populateOpPatterns<arith::RemFOp>(converter, patterns, "__ocml_fmod_f32",
                                     "__ocml_fmod_f64", "__ocml_fmod_f16");
-}
-
-struct ClampFOpConversion : public ConvertOpToLLVMPattern<math::ClampFOp> {
-  using ConvertOpToLLVMPattern::ConvertOpToLLVMPattern;
-  ClampFOpConversion(const LLVMTypeConverter &converter,
-                     amdgpu::Chipset chipset)
-      : ConvertOpToLLVMPattern<math::ClampFOp>(converter), chipset(chipset) {}
 
-  LogicalResult
-  matchAndRewrite(math::ClampFOp op, OpAdaptor adaptor,
-                  ConversionPatternRewriter &rewriter) const override {
-    // V_MED3_F16/F32 only exists in gfx9+ artchitectures
-    if (chipset.majorVersion < 9) {
-      std::string msg =
-          ("pre-gfx9 (gfx" + std::to_string(chipset.majorVersion) +
-           "): V_MED_F16 / V_MED3_F32 not supported.");
-      return rewriter.notifyMatchFailure(op, msg);
-    }
-    rewriter.replaceOpWithNewOp<ROCDL::FMed3Op>(op, op.getType(), op.getValue(),
-                                                op.getMin(), op.getMax());
-    return success();
-  }
-  amdgpu::Chipset chipset;
-};
+  addChipsetDependentPatterns(converter, patterns, chipset);
+}
 
 struct ConvertMathToROCDLPass final
     : impl::ConvertMathToROCDLBase<ConvertMathToROCDLPass> {
@@ -160,8 +170,7 @@ void ConvertMathToROCDLPass::runOnOperation() {
   RewritePatternSet patterns(&getContext());
   LowerToLLVMOptions options(ctx, DataLayout(m));
   LLVMTypeConverter converter(ctx, options);
-  patterns.add<ClampFOpConversion>(converter, *maybeChipset);
-  populateMathToROCDLConversionPatterns(converter, patterns);
+  populateMathToROCDLConversionPatterns(converter, patterns, *maybeChipset);
   ConversionTarget target(getContext());
   target
       .addLegalDialect<BuiltinDialect, func::FuncDialect, vector::VectorDialect,