Fixed non-deterministic GPU intrinsic lowering.

[dfki-jugr]

Summary

The current implementation of the intrinsic lowering phases seemed to be
non-determinsitic accross platforms. Multiple patterns with the same
priority have been matched differently on different platforms (Windows/
Linux) (see https://reviews.llvm.org/D73471). This patch circumvents
this issue by using an adjusted pattern-rewriter benefit to avoid
clashes.

[merge_guards_bot]

Unit tests: fail. 62343 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[rriddle]

This doesn't look an ideal solution. The pattern list is supposed to be deterministic. Can you provide more details here? (ideally with repro instructions)

[mehdi_amini]

This seems like hiding a bug, not fixing it, I'd really like us not to get down this road.

[nicolasvasilache]

Same here, pattern benefits have important use cases. Twiddling them around to make a test pass is not an option.

[herhut]

Let's use dynamicallyIllegalOp instead to disallow the CPU intrinsic explicitly. That would also make the non-determinism go away by not relying on pattern order anymore. And it also captures the actual issue that the intrinsics call is indeed illegal.

[mehdi_amini]

In D73713#1856579, @herhut wrote:

Let's use dynamicallyIllegalOp instead to disallow the CPU intrinsic explicitly. That would also make the non-determinism go away by not relying on pattern order anymore. And it also captures the actual issue that the intrinsics call is indeed illegal.

IIUC: This does not address the underlying issue with the framework though, it seems to still be in the category of "hiding the bug".

[herhut]

In D73713#1858103, @mehdi_amini wrote:

In D73713#1856579, @herhut wrote:

Let's use dynamicallyIllegalOp instead to disallow the CPU intrinsic explicitly. That would also make the non-determinism go away by not relying on pattern order anymore. And it also captures the actual issue that the intrinsics call is indeed illegal.

IIUC: This does not address the underlying issue with the framework though, it seems to still be in the category of "hiding the bug".

No and no. It will not fix the underlying non-determinism issue, so that will have to be addressed. Independently, we should not rely on pattern insertion order for correctness either. So we should exclude the rewriting to LLVM intrinsics that are not available.

So let's get this correct and also fix the underlying issue.

[stella.stamenova]

If this is going to take more than a day or two to fix at this point, I think you should at least XFAIL the tests that are failing on Windows because any runs that include MLIR right now are red and will stay so until your fix lands. You can un-XFAIL them once you verify that they pass with the fix.

[dfki-jugr]

@mehdi_amini I fully get your point and I totally agree that we should not hide underlying bugs. However, you can argue as soon as two pattern matchers have the same benefit their ordering will be undefinied. In this case, the only possible solution you have, is to change the pattern matcher benefit.
@herhut We follow your arguments that we should unbreak the build on Windows asap. As mentioned above, this does not solve the general non-determinism issue. However, even if the ordering would be deterministic, we should not rely on intrinsic magic that might be easily broken by another commit in the future.

[dfki-jugr]

@rriddle I've tried to come up with a minimal example including a new dialect. However, the issue does not occur in simple cases on our testing machines. The easiest way to dive into this issue would be to move line 728 in LowerGPUOpsToNNVMOps.cc to 750:

// move this line
// populateWithGenerated(converter.getDialect()->getContext(), &patterns);
patterns
    .insert<GPUIndexIntrinsicOpLowering<gpu::ThreadIdOp, NVVM::ThreadIdXOp,
                                        NVVM::ThreadIdYOp, NVVM::ThreadIdZOp>,
            GPUIndexIntrinsicOpLowering<gpu::BlockDimOp, NVVM::BlockDimXOp,
                                        NVVM::BlockDimYOp, NVVM::BlockDimZOp>,
            GPUIndexIntrinsicOpLowering<gpu::BlockIdOp, NVVM::BlockIdXOp,
                                        NVVM::BlockIdYOp, NVVM::BlockIdZOp>,
            GPUIndexIntrinsicOpLowering<gpu::GridDimOp, NVVM::GridDimXOp,
                                        NVVM::GridDimYOp, NVVM::GridDimZOp>,
            GPUAllReduceOpLowering, GPUShuffleOpLowering, GPUFuncOpLowering,
            GPUReturnOpLowering>(converter);
patterns.insert<OpToFuncCallLowering<AbsFOp>>(converter, "__nv_fabsf",
                                             "__nv_fabs");
patterns.insert<OpToFuncCallLowering<CeilFOp>>(converter, "__nv_ceilf",
                                             "__nv_ceil");
patterns.insert<OpToFuncCallLowering<CosOp>>(converter, "__nv_cosf",
                                             "__nv_cos");
patterns.insert<OpToFuncCallLowering<ExpOp>>(converter, "__nv_expf",
                                             "__nv_exp");
patterns.insert<OpToFuncCallLowering<TanhOp>>(converter, "__nv_tanhf",
                                              "__nv_tanh");
// to here
populateWithGenerated(converter.getDialect()->getContext(), &patterns);

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h

public:
   /// Builds IR setting ranked memref descriptor ptr
   void setMemRefDescPtr(OpBuilder &builder, Location loc, Value value);
 };
 /// Base class for operation conversions targeting the LLVM IR dialect. Provides
 /// conversion patterns with an access to the containing LLVMLowering for the
 /// purpose of type conversions.
 class LLVMOpLowering : public ConversionPattern {
 public:
+  /// Returns the default benefit value of all LLVM op lowering patterns.
+  static PatternBenefit getDefaultBenefit() { return PatternBenefit(1); }
+
   LLVMOpLowering(StringRef rootOpName, MLIRContext *context,
-                 LLVMTypeConverter &lowering, PatternBenefit benefit = 1);
+                 LLVMTypeConverter &lowering,
+                 PatternBenefit benefit = getDefaultBenefit());
 
 protected:
   // Back-reference to the lowering class, used to call type and function
   // conversions accounting for potential extensions.
   LLVMTypeConverter &lowering;
 };
 
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_STANDARDTOLLVM_CONVERTSTANDARDTOLLVM_H

mlir/include/mlir/IR/PatternMatch.h

public:
 
   static PatternBenefit impossibleToMatch() { return PatternBenefit(); }
   bool isImpossibleToMatch() const { return *this == impossibleToMatch(); }
 
   /// If the corresponding pattern can match, return its benefit.  If the
   // corresponding pattern isImpossibleToMatch() then this aborts.
   unsigned short getBenefit() const;
 
+  /// Increases the current benefit by one (if possible).
+  PatternBenefit increase() const;
+
   bool operator==(const PatternBenefit &rhs) const {
     return representation == rhs.representation;
   }
   bool operator!=(const PatternBenefit &rhs) const { return !(*this == rhs); }
   bool operator<(const PatternBenefit &rhs) const {
     return representation < rhs.representation;
   }
 

mlir/lib/Conversion/GPUCommon/OpToFuncCallLowering.h

 ///   %exp_f32 = std.exp %arg_f32 : f32
 ///
 /// will be transformed into
 ///   llvm.call @__nv_expf(%arg_f32) : (!llvm.float) -> !llvm.float
 template <typename SourceOp>
 struct OpToFuncCallLowering : public LLVMOpLowering {
 public:
   explicit OpToFuncCallLowering(LLVMTypeConverter &lowering_, StringRef f32Func,
-                                StringRef f64Func)
+                                StringRef f64Func,
+                                PatternBenefit benefit = getDefaultBenefit())
       : LLVMOpLowering(SourceOp::getOperationName(),
-                       lowering_.getDialect()->getContext(), lowering_),
+                       lowering_.getDialect()->getContext(), lowering_, benefit),
         f32Func(f32Func), f64Func(f64Func) {}
 
   PatternMatchResult
   matchAndRewrite(Operation *op, ArrayRef<Value> operands,
                   ConversionPatternRewriter &rewriter) const override {
     using LLVM::LLVMFuncOp;
     using LLVM::LLVMType;
 

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp

void mlir::populateGpuToNVVMConversionPatterns(
   patterns.insert<OpToFuncCallLowering<AbsFOp>>(converter, "__nv_fabsf",
                                                "__nv_fabs");
   patterns.insert<OpToFuncCallLowering<CeilFOp>>(converter, "__nv_ceilf",
                                                "__nv_ceil");
   patterns.insert<OpToFuncCallLowering<CosOp>>(converter, "__nv_cosf",
                                                "__nv_cos");
   patterns.insert<OpToFuncCallLowering<ExpOp>>(converter, "__nv_expf",
                                                "__nv_exp");
-  patterns.insert<OpToFuncCallLowering<TanhOp>>(converter, "__nv_tanhf",
-                                                "__nv_tanh");
+  patterns.insert<OpToFuncCallLowering<TanhOp>>(
+      converter, "__nv_tanhf", "__nv_tanh",
+      LLVMOpLowering::getDefaultBenefit().increase());
 }
 
 std::unique_ptr<OpPassBase<gpu::GPUModuleOp>>
 mlir::createLowerGpuOpsToNVVMOpsPass() {
   return std::make_unique<LowerGpuOpsToNVVMOpsPass>();
 }
 
 static PassRegistration<LowerGpuOpsToNVVMOpsPass>
     pass("convert-gpu-to-nvvm", "Generate NVVM operations for gpu operations");

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

void runOnOperation() override {
     patterns.insert<OpToFuncCallLowering<AbsFOp>>(converter, "__ocml_fabs_f32",
                                                   "__ocml_fabs_f64");
     patterns.insert<OpToFuncCallLowering<CeilFOp>>(converter, "__ocml_ceil_f32",
                                                    "__ocml_ceil_f64");
     patterns.insert<OpToFuncCallLowering<CosOp>>(converter, "__ocml_cos_f32",
                                                  "__ocml_cos_f64");
     patterns.insert<OpToFuncCallLowering<ExpOp>>(converter, "__ocml_exp_f32",
                                                  "__ocml_exp_f64");
-    patterns.insert<OpToFuncCallLowering<TanhOp>>(converter, "__ocml_tanh_f32",
-                                                  "__ocml_tanh_f64");
+    patterns.insert<OpToFuncCallLowering<TanhOp>>(
+        converter, "__ocml_tanh_f32", "__ocml_tanh_f64",
+        LLVMOpLowering::getDefaultBenefit().increase());
 
     ConversionTarget target(getContext());
     target.addLegalDialect<LLVM::LLVMDialect, ROCDL::ROCDLDialect>();
     target.addIllegalOp<LLVM::FAbsOp, LLVM::FCeilOp, LLVM::CosOp,
                         LLVM::ExpOp>();
     target.addDynamicallyLegalOp<FuncOp>(
         [&](FuncOp op) { return converter.isSignatureLegal(op.getType()); });
     if (failed(applyPartialConversion(m, target, patterns, &converter)))

mlir/lib/IR/PatternMatch.cpp

 }
 
 unsigned short PatternBenefit::getBenefit() const {
   assert(representation != ImpossibleToMatchSentinel &&
          "Pattern doesn't match");
   return representation;
 }
 
+PatternBenefit PatternBenefit::increase() const {
+  if (representation + 1 < ImpossibleToMatchSentinel)
+    return PatternBenefit(representation + 1);
+  return *this;
+}
+
 //===----------------------------------------------------------------------===//
 // Pattern implementation
 //===----------------------------------------------------------------------===//
 
 Pattern::Pattern(StringRef rootName, PatternBenefit benefit,
                  MLIRContext *context)
     : rootKind(OperationName(rootName, context)), benefit(benefit) {}