Re: r282255 - Fix for r280064 that added options for fp denormals and exceptions.

[Renato Golin via cfe-commits]

On 23 September 2016 at 17:12, Sjoerd Meijer  wrote:
> I don't think I am committing at will:  I thought it was okay to commit 
> directly because it is a simple fix (or should be) for my own  previous 
> half-working patch, but I don't mind going through phab.

A review would have caught many issues with this patch, for example,
the no default issue, and the fact that it has no tests at all.

As a rule of thumb, wait until you get several no-comments-LGTM
reviews in a row in the same area to start committing small patches
before review in that area. Just a few changes isn't enough.


> I am indeed suspecting it is missing default case (probably got confused 
> because e.g. ThreadModel also doesn't have one, at least not there).

Please, also do a build on ARM, and at least try to run check-all, as
that will catch most bugs. And don't forget to add tests.

cheers,
--renato
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r282255 - Fix for r280064 that added options for fp denormals and exceptions.

[Renato Golin via cfe-commits]

On 23 September 2016 at 16:21, Sjoerd Meijer via cfe-commits
 wrote:
> Author: sjoerdmeijer
> Date: Fri Sep 23 10:21:33 2016
> New Revision: 282255
>
> URL: http://llvm.org/viewvc/llvm-project?rev=282255=rev
> Log:
> Fix for r280064 that added options for fp denormals and exceptions.
> These options were forgotten to be copied in setCommandLineOpts.

This broke:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-42vma/builds/12164


> +  Options.FPDenormalType =
> +
> llvm::StringSwitch(CodeGenOpts.FPDenormalMode)
> +  .Case("ieee", llvm::FPDenormal::IEEE)
> +  .Case("preserve-sign", llvm::FPDenormal::PreserveSign)
> +  .Case("positive-zero", llvm::FPDenormal::PositiveZero);

No Default case?



> +  Options.NoTrappingFPMath = CodeGenOpts.NoTrappingMath;

This is not the same as the one above. Are you sure this patch is correct?

Has this been reviewed at all?

Please, don't commit patches at will when nothing is breaking, and put
the patch for review next time.

thanks,
--renato
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r282255 - Fix for r280064 that added options for fp denormals and exceptions.

[Sjoerd Meijer via cfe-commits]

Nothing was breaking, but it wasn't working either (because those options were 
not copied).
I've reverted the patch directly after the first buildbot failure.

I don't think I am committing at will:  I thought it was okay to commit 
directly because it is a simple fix (or should be) for my own  previous 
half-working patch, but I don't mind going through phab.

I am indeed suspecting it is missing default case (probably got confused 
because e.g. ThreadModel also doesn't have one, at least not there).

Cheers,
Sjoerd.

-Original Message-
From: Renato Golin [mailto:renato.go...@linaro.org]
Sent: 23 September 2016 17:00
To: Sjoerd Meijer
Cc: Clang Commits
Subject: Re: r282255 - Fix for r280064 that added options for fp denormals and 
exceptions.

On 23 September 2016 at 16:21, Sjoerd Meijer via cfe-commits 
 wrote:
> Author: sjoerdmeijer
> Date: Fri Sep 23 10:21:33 2016
> New Revision: 282255
>
> URL: http://llvm.org/viewvc/llvm-project?rev=282255=rev
> Log:
> Fix for r280064 that added options for fp denormals and exceptions.
> These options were forgotten to be copied in setCommandLineOpts.

This broke:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-42vma/builds/12164


> +  Options.FPDenormalType =
> +
> llvm::StringSwitch(CodeGenOpts.FPDenormalMode)
> +  .Case("ieee", llvm::FPDenormal::IEEE)
> +  .Case("preserve-sign", llvm::FPDenormal::PreserveSign)
> +  .Case("positive-zero", llvm::FPDenormal::PositiveZero);

No Default case?



> +  Options.NoTrappingFPMath = CodeGenOpts.NoTrappingMath;

This is not the same as the one above. Are you sure this patch is correct?

Has this been reviewed at all?

Please, don't commit patches at will when nothing is breaking, and put the 
patch for review next time.

thanks,
--renato

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r282255 - Fix for r280064 that added options for fp denormals and exceptions.

[Sjoerd Meijer via cfe-commits]

Author: sjoerdmeijer
Date: Fri Sep 23 10:21:33 2016
New Revision: 282255

URL: http://llvm.org/viewvc/llvm-project?rev=282255=rev
Log:
Fix for r280064 that added options for fp denormals and exceptions.
These options were forgotten to be copied in setCommandLineOpts.

Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=282255=282254=282255=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Fri Sep 23 10:21:33 2016
@@ -537,6 +537,12 @@ void EmitAssemblyHelper::CreateTargetMac
   .Case("posix", llvm::ThreadModel::POSIX)
   .Case("single", llvm::ThreadModel::Single);
 
+  Options.FPDenormalType =
+
llvm::StringSwitch(CodeGenOpts.FPDenormalMode)
+  .Case("ieee", llvm::FPDenormal::IEEE)
+  .Case("preserve-sign", llvm::FPDenormal::PreserveSign)
+  .Case("positive-zero", llvm::FPDenormal::PositiveZero);
+
   // Set float ABI type.
   assert((CodeGenOpts.FloatABI == "soft" || CodeGenOpts.FloatABI == "softfp" ||
   CodeGenOpts.FloatABI == "hard" || CodeGenOpts.FloatABI.empty()) &&
@@ -579,6 +585,7 @@ void EmitAssemblyHelper::CreateTargetMac
   Options.LessPreciseFPMADOption = CodeGenOpts.LessPreciseFPMAD;
   Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
   Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
+  Options.NoTrappingFPMath = CodeGenOpts.NoTrappingMath;
   Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
   Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
   Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits