[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

Fri Jul 26 12:05:50 PDT 2019

mibintc marked 3 inline comments as done.
mibintc added a comment.

Thanks for your review >>! In D62731#1601408 <https://reviews.llvm.org/D62731#1601408>, @kpn wrote:

> In D62731#1601310 <https://reviews.llvm.org/D62731#1601310>, @mibintc wrote:
> 
>> I think it would be convenient to have an "unset" setting for the different constrained modes, otherwise you need a boolean that says "no value was provided for this option".  But i'm a frontend person so I may need to have my attitude adjusted.
> 
> 
> What "different constrained modes"? The IRBuilder is either in constrained mode or it isn't. In constrained mode the exception behavior and rounding mode both have defaults, and those defaults can be changed individually without affecting the other setting. The current defaults can also be retrieved if you need something for a function call where you don't want to change it but need an argument anyway. When do you need this "no value provided" setting?

I'm going to rewrite this

> Oh, I'd check the tools/clang/CODE_OWNERS.txt file and add additional appropriate reviewers. Perhaps John McCall and Richard Smith? I don't know who has opinions on how command line options should be handled.

I'd like to fix it more before I add more reviewers

> Do we want the Unix driver to be compatible with gcc? Maybe, maybe not. Opinions, anyone?

Oh, I think you mean something more like the gnuish -fno-except or maybe -fp-model=no-except? instead of -fp-model=except- ok we can get that sorted.

> The documentation request from lebedev.ri isn't in this ticket yet.
> 
> Also, for future historical research purposes I'd cut and paste the relevant portions of outside web pages (like intel.com's) and post them somewhere llvm-ish where they are findable. This ticket, for example, is a good place. Web sites gets reorganized or vanish in full or in part. It's helpful to have some insulation from that over time. I've had to fix compiler bugs that actually were 25 years old before. Yes, 25 years old. Being able to do the research is very helpful.

That's a good idea thanks

> Oh, it may be useful to know that constrained floating point and the FastMathFlags are not mutually exclusive. I don't know if that matters here or not, but you did mention FastMathFlags.

Yes i'm not sure how the fast math command line optoins should interact with the fp-model options, i'll have to dig into that.

================
Comment at: llvm/lib/IR/FPState.cpp:1
+#include "llvm/IR/FPState.h"
+#include "llvm/IR/IRBuilder.h"
----------------
arsenm wrote:
> Missing license header
Thanks @arsenm in the end i believe i won't be adding this file

================
Comment at: llvm/lib/IR/FPState.cpp:73
+  if (IsConstrainedExcept && !IsConstrainedRounding) {
+    // If the rounding mode isn't set explicitly above, then use ebToNearest
+    // as the value when the constrained intrinsic is created
----------------
arsenm wrote:
> eb?
I mean, if the user requested constrained exception via the option -fp-model=except but no rounding mode has been requested (again via command line options) then the rounding mode should be set to "round to nearest".  I'm following a description of how the icc compiler works. I'm afraid that your concise comment "eb?" doesn't convey enough information for me to understand what you mean.  With these further remarks is it clear now? 

================
Comment at: llvm/lib/IR/FPState.cpp:78
+  }
+
+  Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding);
----------------
kpn wrote:
> The IRBuilder already has defaults for exception behavior and rounding. Is it a good idea to duplicate that knowledge here? Worse, what's here is different from what's in the IRBuilder. Why not ask the IRBuilder what its current setting is and use that?
> 
> Would it make sense to have setters/getters, and then have a separate updateBuilder() method? We still don't have a good way to get the #pragma down to the lower levels of clang. The current, unfinished, attempt doesn't work for C++ templates and I'm told other cases as well. An updateBuilder() method could be helpful when moving from one scope to another. But keep in mind that if any constrained fp math is used in a function then the entire function has to be constrained.
> 
> Given that last bit, it may make more sense to have the support somewhere higher level than as a wrapper around the IRBuilder. Maybe in CodeGenFunction or CodeGenModule? I've not spent much time in clang so I'm not sure if that makes sense or not.
Yes I absolutely don't want to duplicate, and I will submit another version of this patch and i'll be removing fpstate*.  I do want to be able to make the fp-model options match the same behavior as icc is using.  One reason i wanted to keep track of the state within a separate object is because i was uncertain if stuff would be going on in the IR builder which would be changing the settings, for whatever reason, and i'd want to put them back into settings specified by the command line options before creating the constrained intrinsics in clang/codegen. 

let me work on this patch more, i just did a quick update to the latest before sending this up.

As far as pragmas versus templates, this is a concern.  Is there something I can read to learn more about the issue?  Pragma's are used in OpenMP so there must be a way to have pragma's interact politely with templates?  Knowing very little, I thought that the pragma would be held as a _Pragma, sort of like a function call, within the intermediate representation e.g. as opposed to a token stream from the preprocessor and I didn't think there would be a problem with templates per se. I'll check with other folks here at Intel. There was  a concern about inlining constrained intrinsics into a function because of your rule about whole function body but nobody mentioned a problem with templates.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731