[mlir] Fix Python bindings tests failure in Debug mode after D98474

[vinograd47]

Summary

Add extra type.isa<FloatType>() check to FloatAttr::get(Type, double) method.
Otherwise it tries to call type.cast<FloatType>(), which fails with assertion in Debug mode.

The !type.isa<FloatType>() case just redirercts the call to FloatAttr::get(Type, APFloat),
which will perform the actual check and emit appropriate error.

[ftynse]

I don't understand this change. What are the other, non-float types that are passed here? Why shouldn't we just assert here (having a FloatAttr expect a FloatType sounds reasonable)?

If Python bindings call this wrong, we can fix the bindings or throw there

[vinograd47]

Python bindings call getChecked method. If I understand correctly, this method should report error instead of assertion. Right now it asserts in Debug and calls method on NULL object in Release.

[vinograd47]

Can we have several validate methods for different builders? Or the validation hook is generated only based on parameters? In that case we might have to perform the same checks in custom builders.

[vinograd47]

@rriddle @ftynse @mehdi_amini Could you please clarify the desired behavior?

Python bindings call FloatType::getChecked method (via C API wrappers) and in case of errors throw exception. The issue is that in Debug mode in case of unsupported type parameter FloatType::getChecked aborts on assertion instead of reporting error.

If I understand correctly, the tblgen for Attributes and Types generates one validate method based on parameters field. The validate method is called in default get and getChecked methods. But if there are custom builders, which are implemented on top of default builders, they won't have separate validate method, which will be called before them. So, they might get unsupported parameters and try to use them in wrong way (like in this case, when custom FloatType::getChecked method calls type.cast<FloatType> without check for type parameter). I'm not sure how to overcome this in elegant way. In this change I've just delegate the validation to default builder, so it will emit the error with message.

[mehdi_amini]

I think that change is fine.
The more interesting thing is that it seems that custom builders should be "safe" in general and not assert on incorrect API uses because they may be called in a "getChecked" context.

[mehdi_amini]

This was broken during the move to ODS I believe: https://reviews.llvm.org/D98474

[mehdi_amini]

I pushed this for now since this is likely fixing this bot: https://lab.llvm.org/buildbot/#/builders/61

mlir/include/mlir/IR/BuiltinAttributes.td

def Builtin_FloatAttr : Builtin_Attr<"Float"> {
   let parameters = (ins AttributeSelfTypeParameter<"">:$type,
                         APFloatParameter<"">:$value);
   let builders = [
     AttrBuilderWithInferredContext<(ins "Type":$type,
                                         "const APFloat &":$value), [{
       return $_get(type.getContext(), type, value);
     }]>,
     AttrBuilderWithInferredContext<(ins "Type":$type, "double":$value), [{
-      if (type.isF64())
+      if (type.isF64() || !type.isa<FloatType>())
         return $_get(type.getContext(), type, APFloat(value));
 
       // This handles, e.g., F16 because there is no APFloat constructor for it.
       bool unused;
       APFloat val(value);
       val.convert(type.cast<FloatType>().getFloatSemantics(),
                   APFloat::rmNearestTiesToEven, &unused);
       return $_get(type.getContext(), type, val);