Return poison from foldBinary/unary if argument(s) is poison. Add ub dialect as dependency to affected dialects (arith, math, spirv, shape).
Add poison materialization to dialects. Add tests for some ops from each dialect.
Not all affected ops are covered as it will involve a huge copypaste.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I think the layering here is a bit problematic. The generic Dialect/CommonFolders.h suddenly depends on the UB dialect.
We are now also suddenly forcing the semantics of poisons on all users of these folders.
I am not sure what the best solution would be, but maybe versions/wrappers of those folders within the UB dialect? That way all op folders/dialects can also opt-into the poison behaviour. This also ensures correctness.
As a sidenote, since your new materializeConstant implementations may now create a ub.poison op, you need to add the UB dialect as a dialect dependency in the TableGen of the respective dialects as well.
CommonFolders.h is used in limited number of places (4) and all of them will benefit from poison IMO, so I'm not sure if it's worth keeping both poison and non-poison version. But I agree that current place may not be the best after introducing dependency on ub dialect, any suggestions for better place?
Regarding runtime dependency on ub dialect, technically, things will still work, as we can only generate poison if it was already present in IR, meaning dialect was already loaded, but that's fragile, yes.
mlir/include/mlir/Dialect/CommonFolders.h | ||
---|---|---|
44 | To break the dependency on the UB dialect, I think we can handle this with a template parameter: template <class AttrElementT, class ElementValueT = typename AttrElementT::ValueType, class CalculationT = function_ref< std::optional<ElementValueT>(ElementValueT, ElementValueT)> class PoisonAttr = void> Attribute constFoldBinaryOpConditional(ArrayRef<Attribute> operands, Type resultType, const CalculationT &calculate) { assert(operands.size() == 2 && "binary op takes two operands"); if (isa_and_nonnull<PoisonAttr>(operands[0])) return operands[0]; if (isa_and_nonnull<PoisonAttr>(operands[1])) return operands[1]; I don't know if the default "void" will work out-of-the-box, but there is likely a trick to have it work here. Now that makes it not use Poison by default, so another way could be a forward decl: namespace ub { class PoisonAttr; } template <class AttrElementT, class ElementValueT = typename AttrElementT::ValueType, class CalculationT = function_ref< std::optional<ElementValueT>(ElementValueT, ElementValueT)> class PoisonAttr = ub::PoisonAttr> And that would make it opt-out for cases where we wouldn't want to depend on the UB dialect... |
LG otherwise, please try to build with SHARED_LIBS=ON to ensure correct build dependencies
There are likely downstream users of these methods (I believe these are quite old), so I do think this would have an impact. At the very least it'd create a link dependency for them.
mlir/include/mlir/Dialect/CommonFolders.h | ||
---|---|---|
44 | I think the first option is a great idea! |
Made PoisonAttr a template parameter as suggested but still left it on by default as I think in most cases this is a desired behavior. Replaced ub dialect include with forward decl, so any potential users will now need to add explicitly dependency on ub dialect or opt out to successfully compile.
mlir/include/mlir/Dialect/Shape/IR/ShapeBase.td | ||
---|---|---|
42 | Shape dialect should not depend on UB dialect. |
Implementation looks good to me, but I rather strongly believe this behaviour should be opt-in rather than opt-out. Otherwise you're by default redefining the semantics of all ops using these. Poison attributes are not the obvious way to handle undefined behaviour but rather a choice in the IR design.
mlir/include/mlir/Dialect/CommonFolders.h | ||
---|---|---|
31 | Can we update the documentation of at least one of these so users can tell whether they need to at least care about PoisonAttr? | |
mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp | ||
977 | nit: use dyn_cast here and all other similar cases that have isa followed by cast |
mlir/include/mlir/Dialect/Shape/IR/ShapeBase.td | ||
---|---|---|
42 | It will depend on it transitively through arith dialect anyway, added ihere explicitly because it is used in materializeConstant. Its there any specific reason why you don't want to handle poison in shape dialect? |
Just for a list of users potentially impacted by opt-out, see: https://github.com/search?q=%22mlir%2FDialect%2FCommonFolders.h%22+language%3AC%2B%2B&type=code
This includes projects like Flang, ClangIR, Circt and IREE.
OK, that's fair. Although, with opt-in most of these users will never know about this poison support and will never update.
There are likely downstream users of these methods (I believe these are quite old), so I do think this would have an impact. At the very least it'd create a link dependency for them.
It's up to them to opt-out, we don't optimize to avoid breaking downstream, we strive for the best default behavior moving forward.
mlir/include/mlir/Dialect/Shape/IR/ShapeBase.td | ||
---|---|---|
42 | materializeConstant does not require a dependent dialect because it only propagate poison and won't create any out-of-thin air. So please remove this everywhere except where there are canonicalization that could break this. |
That's very fair, so maybe I worded this poorly. My concern is not any "churn" downstream in the sense of them requiring actions to keep compiling their code or other refactoring. I like breaking them reguarly as well :-)
Rather, I don't think this is the best default behaviour for such generic functions. My first concern is that always returning poison if any operand is a poison can lead to miscompiles. Obvious cases include things like select operations or other "predicated/masked"-like operations.
Always returning poison therefore can lead to a miscompile. Not propagating poison is only ever a missed optimization, but not an issue of correctness.
It is also not obvious to me that poison semantics, aka deferred UB, is the obvious way that all dialects should be modelling UB. Making it the default would IMO be overfitting to specific kinds of dialects.
I therefore am in favour of the current version of the patch.
This version LGTM but please wait for @jpienaar for the Shape dialect changes/decision and potentially a day for others to comment on it as well.
OK :)
Rather, I don't think this is the best default behaviour for such generic functions. My first concern is that always returning poison if any operand is a poison can lead to miscompiles. Obvious cases include things like select operations or other "predicated/masked"-like operations.
But it's not like we're doing it for any op generically in the folder here: this is "opt-in"!
People don't have to use these helpers...
Always returning poison therefore can lead to a miscompile. Not propagating poison is only ever a missed optimization, but not an issue of correctness.
It is also not obvious to me that poison semantics, aka deferred UB, is the obvious way that all dialects should be modelling UB. Making it the default would IMO be overfitting to specific kinds of dialects.
It's perfectly fine to "overfit" to "specific dialects" when these "specific dialects" includes basically "all" of the dialect we have in-tree :)
(especially when we're only ever talking about the default settings)
That is: I'm not sure I understand really the motivation behind your rebuttal here, I don't quite get how to optimize anything without deferred UB.
mlir/include/mlir/Dialect/CommonFolders.h | ||
---|---|---|
111 | Typo | |
111 | (everywhere else as well) |
@Hardcode84 : don't take this as blocking you from landing, but I'd still like to get to end of the argument with @zero9178 here, this is pretty important for any future direction about how we see UB in MLIR upstream (this "default" deferred UB question in particular may come up all the time!)
That is true, but people are already using these helpers. From that perspective this is not "opt-in", but rather silently changing the semantics of existing functions. The scenario I am scarred about is a fold implementation, using these functions and incorrectly returning a poison attribute.
This is the kind of bug that in my experience is very hard to track down as it carries through all layers of the compilation and successfully generates runnable code as well. Only when running the code and potentially hitting that code path does one notice. Trying to further narrow down to the specific optimization going wrong is doable, but unnecessarily time consuming.
Always returning poison therefore can lead to a miscompile. Not propagating poison is only ever a missed optimization, but not an issue of correctness.
It is also not obvious to me that poison semantics, aka deferred UB, is the obvious way that all dialects should be modelling UB. Making it the default would IMO be overfitting to specific kinds of dialects.It's perfectly fine to "overfit" to "specific dialects" when these "specific dialects" includes basically "all" of the dialect we have in-tree :)
(especially when we're only ever talking about the default settings)
If its a default setting, that's fair.
That is: I'm not sure I understand really the motivation behind your rebuttal here, I don't quite get how to optimize anything without deferred UB.
Let me try to summarize my position: I am warry of silently redefining a function already used in optimizations to potentially create hard to track down miscompiles. If these were not already used, but newly defined or there was an effective way of communicating to users about the change of semantics it'd be easier. Build breakage with new APIs tend to get people to look throught the code and check what the right thing to do is e.g. A PSA would fit this definition as well but that is more likely to be ignored and more effort than just not silently redefining the function.
Regarding handling of UB in the future: I guess my fear was mostly about the silently redefining behaviour in a detremental way. I think the ub dialect is the way forward and a good default and that users will end up having to care about it, worst-case when they use in-tree dialects like arith.
It's not critical so I can wait. Regarding default value, I'm fine with either variant.
+1, this really resonates with me and is the reason why I approved the patch in the current form. While we do not promise to keep any downstream code working forever, this could result in surprising and difficult to track down change of behavior, while the alternative of having a few more functions is low cost to pay for the upstream IMO.
It can't be silent: they would have a build breakage and decide to depend on the UB dialect!
(also back to the previous point about breaking users)
That is: I'm not sure I understand really the motivation behind your rebuttal here, I don't quite get how to optimize anything without deferred UB.
Let me try to summarize my position: I am warry of silently redefining a function already used in optimizations to potentially create hard to track down miscompiles.
I agree with this concern, I assumed here that this would be a build breakage (we forward declare the UB attribute, folks must include the UB dialect and link to it)
What I was concerned about was downstream dialects that use these fold helpers -- with no code change downstream, they would end up propagating poison values created by upstream rewrites. Right now we do not produce poison in any folds/canon patterns AFAIK, but if this changes at some point, it seems risky to me without any explicit opt-in mechanism for everything that ends up consuming results of these upstream dialects.
Sure, we all agree on this, my point was that we should make the default be to use poison and make it a build break so that what you're describing does not exist.
True, while this is likely to lead to a build break, I don't think the prior version was 1) reliable and 2) descriptive enough to warn users. Linkage wise, they're unlikely to get an error if already transitively linking against the UB dialect (e.g. the arith dialect). Compilation wise, the error is also not very helpful: https://godbolt.org/z/5YW439Pva One gets a template error 5 layers deeper saying that ub::PoisonAttr is an incomplete type. I'd argue the reflex of most C++ programms in such a scenario is to include the header defining that type. I have rather little trust that people would see this error, go through the backtrace to find that its coming from their usage of these fold methods, and then start thinking whether returning poison in their given uses is the right thing or not.
They'll rather just try and get it to build, run their tests and sign-off only for it to potentially become a problem in the future.
Besides the redfining issue, there's also an argument one can make that the default general function should be safe by default. Correct me if I am wrong, but the argument for making the poison behaviour the default is that the empirical usage upstream of these folders has so far been safe, rather than it being a conscious design decision. I'd rather have a safe-by-default design.
OK but now I feel we're goal post shifting a bit from the previous objections (silent miscompile).
Linkage wise, they're unlikely to get an error if already transitively linking against the UB dialect (e.g. the arith dialect).
Sure, the goal is to have a compile-time failure, I didn't mention a "linker failure" anywhere. Before getting to the linker, you need to include the headers.
Compilation wise, the error is also not very helpful: https://godbolt.org/z/5YW439Pva One gets a template error 5 layers deeper saying that ub::PoisonAttr is an incomplete type. I'd argue the reflex of most C++ programms in such a scenario is to include the header defining that type. I have rather little trust that people would see this error, go through the backtrace to find that its coming from their usage of these fold methods, and then start thinking whether returning poison in their given uses is the right thing or not.
They'll rather just try and get it to build, run their tests and sign-off only for it to potentially become a problem in the future.
Sure, we can argue that's not ideal, but we're very far from what you were discussing before!
Besides the redfining issue, there's also an argument one can make that the default general function should be safe by default. Correct me if I am wrong, but the argument for making the poison behaviour the default is that the empirical usage upstream of these folders has so far been safe, rather than it being a conscious design decision. I'd rather have a safe-by-default design.
"safe-by-default" is a good principle, it cannot be the only driving principle, in particular if "safe" goes against "useful". There is a balance and good APIs should be "easy to use, hard to misuse".
Just going for safe here cannot be a satisfactory answer.
(especially since the "safe" aspect of it is quite... arguable really)
How does the build break looks like for the existing users with forward declaration and default to poison?
[1/27] Building CXX object tools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj FAILED: tools/mlir/lib/Dialect/Math/IR/CMakeFiles/obj.MLIRMathDialect.dir/MathOps.cpp.obj C:\PROGRA~2\MICROS~2\2022\BUILDT~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\cl.exe /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_HARDENED_MODE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\projs\llvm\llvm-build\tools\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-build\include -ID:\projs\llvm\llvm-project\llvm\include -ID:\projs\llvm\llvm-project\mlir\include -ID:\projs\llvm\llvm-build\tools\mlir\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /O2 /Ob2 -std:c++17 -MD /EHs-c- /GR- -UNDEBUG /showIncludes /Fotools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj /Fdtools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\ /FS -c D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp D:\projs\llvm\llvm-project\mlir\include\mlir/IR/BuiltinAttributes.h(359): warning C4996: 'std::complex<llvm::APFloat>::complex': warning STL4037: The effect of instantiating the template std::complex for any type other than float, double, or long double is unspecified. You can define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to suppress this warning. D:\projs\llvm\llvm-project\llvm\include\llvm/Support/Casting.h(657): error C2027: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(526): note: see reference to function template instantiation 'decltype(auto) llvm::dyn_cast<mlir::ub::PoisonAttr,mlir::Attribute>(From &)' being compiled with [ From=mlir::Attribute ] D:\projs\llvm\llvm-project\llvm\include\llvm/Support/Casting.h(655): error C3487: 'void': all return expressions must deduce to the same type: previously it was 'To' with [ To=mlir::ub::PoisonAttr ] D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(526): error C2027: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(526): error C2027: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(526): error C2451: a conditional expression of type 'To' is not valid with [ To=mlir::ub::PoisonAttr ] D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(526): note: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(527): error C2039: 'PoisonOp': is not a member of 'mlir::ub' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(25): note: see declaration of 'mlir::ub' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(527): error C2065: 'PoisonOp': undeclared identifier D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(527): error C2672: 'mlir::OpBuilder::create': no matching overloaded function found D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Builders.h(491): note: could be 'OpTy mlir::OpBuilder::create(mlir::Location,Args &&...)' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(527): note: 'mlir::OpBuilder::create': invalid template argument for 'OpTy', type expected ninja: build stopped: subcommand failed.
VS 2022
[1/27] Building CXX object tools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj FAILED: tools/mlir/lib/Dialect/Math/IR/CMakeFiles/obj.MLIRMathDialect.dir/MathOps.cpp.obj C:\PROGRA~2\MICROS~2\2022\BUILDT~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\cl.exe /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_HARDENED_MODE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\projs\llvm\llvm-build\tools\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-build\include -ID:\projs\llvm\llvm-project\llvm\include -ID:\projs\llvm\llvm-project\mlir\include -ID:\projs\llvm\llvm-build\tools\mlir\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /O2 /Ob2 -std:c++17 -MD /EHs-c- /GR- -UNDEBUG /showIncludes /Fotools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj /Fdtools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\ /FS -c D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp D:\projs\llvm\llvm-project\mlir\include\mlir/IR/BuiltinAttributes.h(359): warning C4996: 'std::complex<llvm::APFloat>::complex': warning STL4037: The effect of instantiating the template std::complex for any type other than float, double, or long double is unspecified. You can define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to suppress this warning. D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Attributes.h(406): error C2027: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Attributes.h(399): note: while compiling class template member function 'bool llvm::CastInfo<mlir::ub::PoisonAttr,const T,void>::isPossible(mlir::Attribute)' with [ T=mlir::Attribute ] D:\projs\llvm\llvm-project\llvm\include\llvm/Support/Casting.h(549): note: see reference to function template instantiation 'bool llvm::CastInfo<mlir::ub::PoisonAttr,const T,void>::isPossible(mlir::Attribute)' being compiled with [ T=mlir::Attribute ] D:\projs\llvm\llvm-project\llvm\include\llvm/Support/Casting.h(548): note: see reference to class template instantiation 'llvm::CastInfo<mlir::ub::PoisonAttr,const T,void>' being compiled with [ T=mlir::Attribute ] D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(186): note: see reference to function template instantiation 'bool llvm::isa<mlir::ub::PoisonAttr,T>(const From &)' being compiled with [ T=mlir::Attribute, From=mlir::Attribute ] D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(338): note: see reference to function template instantiation 'mlir::Attribute mlir::constFoldUnaryOpConditional<mlir::FloatAttr,mlir::FloatAttr::ValueType,mlir::ub::PoisonAttr,mlir::math::AtanOp::fold::<lambda_1>>(llvm::ArrayRef<T>,CalculationT &&)' being compiled with [ T=mlir::Attribute, CalculationT=mlir::math::AtanOp::fold::<lambda_1> ] D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(59): note: see reference to function template instantiation 'mlir::Attribute mlir::constFoldUnaryOpConditionalPoison<mlir::FloatAttr,mlir::FloatAttr::ValueType,mlir::math::AtanOp::fold::<lambda_1>>(llvm::ArrayRef<T>,CalculationT &&)' being compiled with [ T=mlir::Attribute, CalculationT=mlir::math::AtanOp::fold::<lambda_1> ] D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Attributes.h(406): error C3861: 'classof': identifier not found ninja: build stopped: subcommand failed.
forgot to comment out materializer
Thanks!
To me seeing:
D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Attributes.h(406): error C2027: use of undefined type 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(26): note: see declaration of 'mlir::ub::PoisonAttr' D:\projs\llvm\llvm-project\mlir\include\mlir/IR/Attributes.h(399): note: while compiling class template member function 'bool llvm::CastInfo<mlir::ub::PoisonAttr,const T,void>::isPossible(mlir::Attribute)' with
Is on-par with what I expect people to see and trigger them to figure out whether this is what they want.
Worth mentioning also that even if they don't think further and just add the right includes, it'll only kick in if they have poison in their IR already.
@zero9178 : what kind of error would you want to see here? Do you have a suggestion that would make this harder to misuse for the users while preserving the "right" default in general?
I think a verbose message would be a good compromise. One could detect it with something liek https://devblogs.microsoft.com/oldnewthing/20190710-00/?p=102678 (usual sfinae pattern used on sizeof to check whether the type is complete) if that is not too much effort.
The error message could read something like "PoisonAttr must be a complete type. If any of the operands are an attribute of this kind it will be propagated to the result.".
This also has the advantage of bringing the error location right to the offending function and it being a non-standard C++ error that users shouldn't have a reflex fix for.
I think this would be an okay compromise so that we can land this
Would you be more comfortable with it if instead of doing an one-of thing here, we first added a type completeness check to STLExtras?
I'm fine with it as long as ensuring it works consistently across different compilers will not be my responsibility :)
Also it will probably add (small) compilation overhead for everyone, I'm fine with this too, but others may argue.
Now build failure looks like this:
[1/27] Building CXX object tools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj FAILED: tools/mlir/lib/Dialect/Math/IR/CMakeFiles/obj.MLIRMathDialect.dir/MathOps.cpp.obj C:\PROGRA~2\MICROS~2\2022\BUILDT~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\cl.exe /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_HARDENED_MODE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -ID:\projs\llvm\llvm-build\tools\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR -ID:\projs\llvm\llvm-build\include -ID:\projs\llvm\llvm-project\llvm\include -ID:\projs\llvm\llvm-project\mlir\include -ID:\projs\llvm\llvm-build\tools\mlir\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /O2 /Ob2 -std:c++17 -MD /EHs-c- /GR- -UNDEBUG /showIncludes /Fotools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\MathOps.cpp.obj /Fdtools\mlir\lib\Dialect\Math\IR\CMakeFiles\obj.MLIRMathDialect.dir\ /FS -c D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp D:\projs\llvm\llvm-project\mlir\include\mlir/IR/BuiltinAttributes.h(359): warning C4996: 'std::complex<llvm::APFloat>::complex': warning STL4037: The effect of instantiating the template std::complex for any type other than float, double, or long double is unspecified. You can define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to suppress this warning. D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(200): error C2338: static_assert failed: 'PoisonAttr is undefined, either add a dependency on UB dialect or pass void as template argument to opt-out from poison semantics.' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(59): note: see reference to function template instantiation 'mlir::Attribute mlir::constFoldUnaryOpConditional<mlir::FloatAttr,mlir::FloatAttr::ValueType,mlir::ub::PoisonAttr,mlir::math::AtanOp::fold::<lambda_1>>(llvm::ArrayRef<T>,CalculationT &&)' being compiled with [ T=mlir::Attribute, CalculationT=mlir::math::AtanOp::fold::<lambda_1> ] D:\projs\llvm\llvm-project\mlir\include\mlir/Dialect/CommonFolders.h(127): error C2338: static_assert failed: 'PoisonAttr is undefined, either add a dependency on UB dialect or pass void as template argument to opt-out from poison semantics.' D:\projs\llvm\llvm-project\mlir\lib\Dialect\Math\IR\MathOps.cpp(82): note: see reference to function template instantiation 'mlir::Attribute mlir::constFoldBinaryOpConditional<mlir::FloatAttr,mlir::FloatAttr::ValueType,mlir::ub::PoisonAttr,mlir::math::Atan2Op::fold::<lambda_1>>(llvm::ArrayRef<T>,CalculationT &&)' being compiled with [ T=mlir::Attribute, CalculationT=mlir::math::Atan2Op::fold::<lambda_1> ] ninja: build stopped: subcommand failed.
Can we update the documentation of at least one of these so users can tell whether they need to at least care about PoisonAttr?