Thanks for your review. I put some quick replies below. I'll work on an update to this.
> -----Original Message----- > From: Kevin P. Neal via Phabricator [mailto:revi...@reviews.llvm.org] > Sent: Thursday, July 25, 2019 1:09 PM > To: Blower, Melanie <melanie.blo...@intel.com>; chandl...@gmail.com > Cc: mgo...@gentoo.org; hiradi...@msn.com; wuz...@cn.ibm.com; Wang, > Pengfei <pengfei.w...@intel.com>; lebedev...@gmail.com; > cameron.mcina...@nyu.edu; llvm-comm...@lists.llvm.org; Rice, Michael P > <michael.p.r...@intel.com>; Kaylor, Andrew <andrew.kay...@intel.com>; > kevin.n...@sas.com; cfe-commits@lists.llvm.org; paul.robin...@sony.com; > peter.wal...@arm.com > Subject: [PATCH] D62731: [RFC] Add support for options -fp-model= and -fp- > speculation= : specify floating point behavior > > kpn added a comment. > > 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? [Blower, Melanie] I was thinking it would be convenient to have null values for RoundingMode and ExceptionBehavior but I'll work on the patch more in this area > > 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. [Blower, Melanie] Good idea but I'm not quite ready yet > > Do we want the Unix driver to be compatible with gcc? Maybe, maybe not. > Opinions, anyone? [Blower, Melanie] For Intel's icc compiler, we support options describing fp-model in both Linux and Windows. Our customers find it useful. I want to add it both places. Gcc doesn't support these options. > > 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. [Blower, Melanie] good idea, I will address that, adding it to this patch description? Or inline as comments in the source file itself? (I may be able to compete with you on oldest bug fixed...) > > 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. [Blower, Melanie] yes I worry about that > > > > ================ > Comment at: llvm/lib/IR/FPState.cpp:78 > + } > + > + Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding); > ---------------- > 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? [Blower, Melanie] I was/am/ trying to translate the fp-model command line options semantics. I will look at this again. > > 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. [Blower, Melanie] hmm > > 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. > > > Repository: > rL LLVM > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D62731/new/ > > https://reviews.llvm.org/D62731 > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
