This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ub] Add poison support to CommonFolders.h
ClosedPublic

Authored by Hardcode84 on Aug 28 2023, 12:57 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Hardcode84 created this revision.Aug 28 2023, 12:57 PM
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Hardcode84 requested review of this revision.Aug 28 2023, 12:57 PM

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.

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.

mehdi_amini added inline comments.Aug 28 2023, 3:47 PM
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

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?

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!
You can make it work with the default void by just wrapping these two ifs into a if constexpr (!std::is_void_v<PoisonAttr>).

make poison handling optional, add ub dialect as runtime dependency

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.

jpienaar requested changes to this revision.Aug 29 2023, 4:51 AM
jpienaar added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeBase.td
42

Shape dialect should not depend on UB dialect.

This revision now requires changes to proceed.Aug 29 2023, 4:51 AM

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

Hardcode84 added inline comments.Aug 29 2023, 5:00 AM
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?

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.

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.

make poison opt-in

Hardcode84 marked 2 inline comments as done.Aug 29 2023, 6:26 AM

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.

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.

kuhar accepted this revision.Aug 29 2023, 7:41 AM

Awesome!

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.
(and if something does not show up with a grep in the dialect, it should be documented as well)

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.

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.

remove runtime dependecy on ub dialect

Hardcode84 marked an inline comment as done.Aug 30 2023, 7:46 AM
zero9178 accepted this revision.Aug 30 2023, 8:19 AM

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.

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.

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 :-)

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!)

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.

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 :-)

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...

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.

rebase, fix typo

Hardcode84 marked 2 inline comments as done.Aug 31 2023, 5:13 AM

@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!)

It's not critical so I can wait. Regarding default value, I'm fine with either variant.

kuhar added a comment.Aug 31 2023, 7:45 AM

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.

+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.

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.

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 :-)

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...

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.

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)

kuhar added a comment.Aug 31 2023, 1:18 PM

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.

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 :-)

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...

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.

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.

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.

zero9178 added a comment.EditedSep 1 2023, 12:13 AM

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.

mehdi_amini added a comment.EditedSep 1 2023, 4:22 PM

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.

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)

So, what the final decision?

mehdi_amini added a comment.EditedSep 5 2023, 1:11 PM

So, what the final decision?

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

This comment was removed by mehdi_amini.

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?

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

I'm not a big fan of those SFINAE tricks.

kuhar added a comment.Sep 6 2023, 7:38 AM

I'm not a big fan of those SFINAE tricks.

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 not a big fan of those SFINAE tricks.

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.

kuhar added a comment.Sep 6 2023, 9:00 AM

I'm not a big fan of those SFINAE tricks.

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.

Here you go: https://github.com/llvm/llvm-project/pull/65495

kuhar added a comment.Sep 6 2023, 11:11 AM

@Hardcode84 I've just landed llvm::is_incomplete_v

better error message

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.
mehdi_amini accepted this revision.Sep 6 2023, 6:25 PM

Sweet!

zero9178 accepted this revision.Sep 7 2023, 12:56 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 7 2023, 3:30 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.