[PATCH] D136176: Implement support for option 'fexcess-precision'.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:320
 BENIGN_ENUM_LANGOPT(FPEvalMethod, FPEvalMethodKind, 2, FEM_UnsetOnCommandLine, "FP type used for floating point arithmetic")
+BENIGN_ENUM_LANGOPT(FPPrecisionMode, FPExcessPrecisionModeKind, 2, FPP_Standard, "FP precision used for floating point arithmetic")
 LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field type alignment")
----------------
andrew.w.kaylor wrote:
> A lot of people mix up precision and accuracy, so I think this description is likely to be ambiguous. I'd suggest "Intermediate truncation behavior for floating point arithmetic"
Unfortunately, I'm not sure this is a "benign" language option — we haven't implemented this yet, but constant evaluation is actually supposed to use the same excess-precision semantics as normal evaluation, which means that in weird C++ corner cases, it can absolutely change things like template specialization identity (because you can have floating-point template arguments in recent C++ standards).


================
Comment at: clang/include/clang/Basic/LangOptions.h:300
+    FPP_Fast,
+    FPP_None
+  };
----------------
zahiraam wrote:
> zahiraam wrote:
> > andrew.w.kaylor wrote:
> > > Is FPP_None somehow the same as fexcess-precision=16? If not, what does it mean?
> > Yes, maybe this is confusing.  How about if we make "none" means disable excess precision and not use "16"? If you agree with this, then I will edit the description of the issue above.
> > Is FPP_None somehow the same as fexcess-precision=16? If not, what does it mean?
> 
> I remember that @rjmccall wanted to have 16 a legal value so that we are compatible with GCC. Done that here.
My memory is that we investigated and found that GCC was using `-fexcess-precision=16` to mean "don't emit `_Float16` operations with excess precision".  I don't really like this spelling — it seems to assume that there's only one interesting kind of excess-precision arithmetic in play for any particular target — but it's what GCC has specced out, so if we're going to use `-fexcess-precision` as the driver argument, we need to play by their rules.

With that said, I think our internal representation (and maybe the -cc1 arguments?) should be very explicit about what type is affected.  So the LangOpt should be something like `Float16ExcessPrecision`, and the driver should turn e.g. `-fexcess-precision=16` into `-ffloat16-excess-precision=none`.


================
Comment at: clang/test/CodeGen/X86/fexcess-precise.c:89
+// CHECK-EXT-NEXT:    [[EXT1:%.*]] = fpext half [[TMP1]] to float
+// CHECK-EXT-NEXT:    [[MUL:%.*]] = fmul float [[EXT]], [[EXT1]]
+// CHECK-EXT-NEXT:    [[TMP2:%.*]] = load half, ptr [[C_ADDR]], align 2
----------------
andrew.w.kaylor wrote:
> This is not what I'd expect to see. I'd expect the operations to use the half type with explicit truncation inserted where needed.
Are you suggesting that the frontend emit `half` operations normally, with some intrinsic to force `half` precision on casts and assignments, and that a backend pass would aggressively promote operations between those intrinsics?  I think that would be a pretty error-prone representation, both in terms of guaranteeing the use of excess precision in some situations (and thus getting stable behavior across compiler releases) and guaranteeing truncation in others (and thus preserving correctness).  The frontend would have to carefully emit intrinsics in a bunch of places or else default to introducing a bug.


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

https://reviews.llvm.org/D136176