[PATCH] D139902: IR: Add nofpclass parameter attribute

Wed Dec 21 01:11:22 PST 2022

pengfei added inline comments.

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14781
   case X86::BI__builtin_ia32_reduce_fmax_ph128: {
+    /// FIXME: This is broken
     Function *F =
----------------
arsenm wrote:
> pengfei wrote:
> > arsenm wrote:
> > > pengfei wrote:
> > > > arsenm wrote:
> > > > > pengfei wrote:
> > > > > > What's broken here? Intrinsics are usually mapped directly to instructions which have different assumptions on the values.
> > > > > Accidental diff. This is forcibly setting fast math flags on the builtins regardless of the fp mode, and it's not using a flag guard. This is going to infect later built instructions with wrong fast math flags
> > > > Target specific intrinsics are flexible. Some of them imply specific fast math flags. Even compiled without fast math flags, passing e.g., NaNs in this case is UB.
> > > Regardless of the behavior of the builtins, this is still not using a flag guard. It's making any instructions inserted with this Builder also have the fast math flags.
> > Agreed. Thanks for the explanation!
> https://godbolt.org/z/PTseYq3Y9 demonstrates the problem. The fadd ends up with nnan implying that c cannot be nan which is not implied by the source
Yeah, but it is only a problem when user uses the builtin directly. This is not encouraged. Anyway, I put a patch D140467 for it. It's still nice to have.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139902/new/

https://reviews.llvm.org/D139902