Page MenuHomePhabricator

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

Authored by vinograd47 on Mar 17 2021, 1:58 AM.

Details

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.

Diff Detail

Event Timeline

vinograd47 created this revision.Mar 17 2021, 1:58 AM
vinograd47 requested review of this revision.Mar 17 2021, 1:58 AM

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

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.

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.

@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 accepted this revision.Mar 18 2021, 10:52 AM

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.

This revision is now accepted and ready to land.Mar 18 2021, 10:52 AM

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