[mlir][UBToLLVM] Do not arbitrarily restrict input types

[zero9178]

Summary

The lowering pattern is currently restricted to integer, float and index types.
This is seemingly arbitrary, as ub.poison works for any input type. The lowering should therefore also work with any type that can be converted using the type converter.

This patch therefore simply removes that condition and adds a test ensuring that this works.

[Hardcode84]

My motivation for limiting this pattern was to avoid converting user types as user might have different lowering for poison. But it is purely theoretical, I don't have any specific examples.

[zero9178]

Restrict pattern to #ub.poison only

[zero9178]

In D158982#4621286, @Hardcode84 wrote:

My motivation for limiting this pattern was to avoid converting user types as user might have different lowering for poison. But it is purely theoretical, I don't have any specific examples.

I now changed the implementation to only perform the folding if using the default #ub.poison attribute. In this case this pattern should be completely correct as it is fully poisoned. The current lowering would be incorrect for any other attribute.
Any other implementation of a PoisonAttrInterface is not handled by the pattern and almost certainly wants a custom lowering.

If threoretically a user might want a different lowering, they could just not use this pass, or register this pattern as part of their UB lowering.

[kuhar]

In D158982#4621370, @zero9178 wrote:

In D158982#4621286, @Hardcode84 wrote:

My motivation for limiting this pattern was to avoid converting user types as user might have different lowering for poison. But it is purely theoretical, I don't have any specific examples.

I now changed the implementation to only perform the folding if using the default #ub.poison attribute. In this case this pattern should be completely correct as it is fully poisoned. The current lowering would be incorrect for any other attribute.
Any other implementation of a PoisonAttrInterface is not handled by the pattern and almost certainly wants a custom lowering.

If threoretically a user might want a different lowering, they could just not use this pass, or register this pattern as part of their UB lowering.

This makes sense to me. We should probably extend the documentation soon to explain these semantics, similar to https://mlir.llvm.org/docs/Rationale/SideEffectsAndSpeculation/ to avoid future ambiguities here. And I'm sure it will take a while to iron everything out.

mlir/lib/Conversion/UBToLLVM/UBToLLVM.cpp

45–50

ubernit: I'd prefer braces around ifs with multi-line body

[Hardcode84]

LGTM (FYI, we have a similar pattern in UBToSPIRV)

mlir/lib/Conversion/UBToLLVM/UBToLLVM.cpp

 
 //===----------------------------------------------------------------------===//
 // PoisonOpLowering
 //===----------------------------------------------------------------------===//
 
 LogicalResult
 PoisonOpLowering::matchAndRewrite(ub::PoisonOp op, OpAdaptor adaptor,
                                   ConversionPatternRewriter &rewriter) const {
-  Type origType = op.getType();
-  if (!origType.isIntOrIndexOrFloat())
-    return rewriter.notifyMatchFailure(
-        op, [&](Diagnostic &diag) { diag << "unsupported type " << origType; });
+  if (!isa<ub::PoisonAttr>(op.getValue())) {
+    return rewriter.notifyMatchFailure(op, [&](Diagnostic &diag) {
+      diag << "pattern can only convert op with '"
+           << ub::PoisonAttr::getMnemonic() << "' poison value";
+    });
+  }
 
-  Type resType = getTypeConverter()->convertType(origType);
-  if (!resType)
+  Type resType = getTypeConverter()->convertType(op.getType());
+  if (!resType) {
     return rewriter.notifyMatchFailure(op, [&](Diagnostic &diag) {
-      diag << "failed to convert result type " << origType;
+      diag << "failed to convert result type " << op.getType();
     });
+  }
 
   rewriter.replaceOpWithNewOp<LLVM::PoisonOp>(op, resType);
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Pass Definition
 //===----------------------------------------------------------------------===//

mlir/test/Conversion/UBToLLVM/ub-to-llvm.mlir

 // RUN: mlir-opt -pass-pipeline="builtin.module(func.func(convert-ub-to-llvm))" %s -split-input-file | FileCheck %s
 
 // Same below, but using the `ConvertToLLVMPatternInterface` entry point
 // and the generic `convert-to-llvm` pass.
 // RUN: mlir-opt --convert-to-llvm="filter-dialects=ub" --split-input-file %s | FileCheck %s
 
 // CHECK-LABEL: @check_poison
 func.func @check_poison() {
 // CHECK: {{.*}} = llvm.mlir.poison : i64
   %0 = ub.poison : index
 // CHECK: {{.*}} = llvm.mlir.poison : i16
   %1 = ub.poison : i16
 // CHECK: {{.*}} = llvm.mlir.poison : f64
   %2 = ub.poison : f64
+// CHECK: {{.*}} = llvm.mlir.poison : !llvm.ptr
+  %3 = ub.poison : !llvm.ptr
   return
 }