[PATCH] Add support for generating MIPS legacy NaN
Vladimir Radosavljevic
vladimir.radosavljevic at rt-rk.com
Thu Feb 26 08:22:10 PST 2015
================
Comment at: lib/AST/ExprConstant.cpp:7599-7603
@@ +7598,7 @@
+ } else {
+ // This case will be triggered for Mips legacy encoding.
+ if (SNaN)
+ Result = llvm::APFloat::getQNaN(Sem, false, &fill);
+ else
+ Result = llvm::APFloat::getSNaN(Sem, false, &fill);
+ }
----------------
dsanders wrote:
> Could you add a comment explaining the relevant history and why that leads to QNaN and SNaN being reversed here. It won't be obvious to most readers.
Done.
================
Comment at: lib/Basic/Targets.cpp:5679
@@ +5678,3 @@
+ bool isNan2008() const override {
+ return IsNan2008 || isNaN2008Default();
+ }
----------------
petarj wrote:
> dsanders wrote:
> > petarj wrote:
> > > vradosavljevic wrote:
> > > > dsanders wrote:
> > > > > This isn't quite right. It should just be IsNan2008 since IsNan2008 is initialized with the result of isNaN2008Default().
> > > > When you generating code for mips32r6/mips64r6 with option -mnan=legacy, IsNan2008 will be set to false.
> > > > In function handleTargetFeatures there are lines that change that.
> > > > else if (*it == "-nan2008")
> > > > IsNan2008 = false;
> > > > Because of that, I use the result of isNaN2008Default().
> > > We should not allow that case. Clang should (at least) issue a warning if "-mnan=legacy" is passed for the revisions that do not support the NaN legacy encoding.
> > Agreed. I'd prefer an error* since the user is requesting an impossible combination of options but it should at least warn instead of quietly ignoring the user.
> >
> > *Ideally with a suggestion for fixing it, e.g. "error: IEEE754(2008) compliant NaN's are mandatory on Mips32r6. Did you mean -mnan=2008?"
> I would also prefer an error, but gcc seems to produce a warning only.
> Furthermore, we should do similar for "-mnan=2008" and earlier mips revisions. But this all can be part of another patch, not this one necessary.
Done.
================
Comment at: lib/Basic/Targets.cpp:5679
@@ +5678,3 @@
+ bool isNan2008() const override {
+ return IsNan2008 || isNaN2008Default();
+ }
----------------
vradosavljevic wrote:
> petarj wrote:
> > dsanders wrote:
> > > petarj wrote:
> > > > vradosavljevic wrote:
> > > > > dsanders wrote:
> > > > > > This isn't quite right. It should just be IsNan2008 since IsNan2008 is initialized with the result of isNaN2008Default().
> > > > > When you generating code for mips32r6/mips64r6 with option -mnan=legacy, IsNan2008 will be set to false.
> > > > > In function handleTargetFeatures there are lines that change that.
> > > > > else if (*it == "-nan2008")
> > > > > IsNan2008 = false;
> > > > > Because of that, I use the result of isNaN2008Default().
> > > > We should not allow that case. Clang should (at least) issue a warning if "-mnan=legacy" is passed for the revisions that do not support the NaN legacy encoding.
> > > Agreed. I'd prefer an error* since the user is requesting an impossible combination of options but it should at least warn instead of quietly ignoring the user.
> > >
> > > *Ideally with a suggestion for fixing it, e.g. "error: IEEE754(2008) compliant NaN's are mandatory on Mips32r6. Did you mean -mnan=2008?"
> > I would also prefer an error, but gcc seems to produce a warning only.
> > Furthermore, we should do similar for "-mnan=2008" and earlier mips revisions. But this all can be part of another patch, not this one necessary.
> Done.
Ok. This will be part of the next patch.
================
Comment at: test/CodeGen/builtin-nan-legacy.c:2-3
@@ +1,4 @@
+// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -emit-llvm -S -o %t %s
+// RUN: grep 'float 0x7FF4000000000000, float 0x7FF8000000000000' %t
+// RUN: grep 'double 0x7FF4000000000000, double 0x7FF8000000000000' %t
+
----------------
dsanders wrote:
> New tests should be using FileCheck rather than grep.
Done.
http://reviews.llvm.org/D7882
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list