- User Since
- Jan 2 2013, 1:50 PM (367 w, 2 d)
I don't know if there were other reviewers who haven't commented on how you addressed their concerns, but this looks good to me.
Thu, Jan 16
Could you make this a standalone patch that only modifies the table without adding the FP_CONTRACT support? Then the other patch can just be refactored to use this.
Fri, Jan 10
Thu, Jan 9
Wed, Jan 8
Mon, Jan 6
Fri, Jan 3
Mon, Dec 23
Dec 18 2019
Dec 17 2019
Dec 13 2019
I'm late jumping into the discussion of these patches and may not have seen all comments, so go easy on me if I say something that has already been covered.
Dec 12 2019
Dec 11 2019
Dec 4 2019
I'm afraid we may have made a mess of this for X86 in D70214. We noticed the problem that you're fixing here, but I didn't realize you had a solution for it. I guess I hadn't looked at this patch. I'm sorry about that. Your solution looks good to me.
This was fixed in a different way.
Can this patch be abandoned?
I think we've agreed that this isn't a good use of tokens. The operand bundle approach may still be useful, but that can be attempted in a new revision.
Nov 27 2019
To clarify, I was suggesting a single intrinsic with quiet/signaling encoded in the predicate. I think the IR definition is a bit cleaner this way and it maps to both the IEEE specification and at least the most common way (I think) this is implemented in x86 hardware and probably others. That said, if doing it this way makes the implementation more complicated I could go along with separate intrinsics and ISD opcodes.
Nov 26 2019
Nov 25 2019
I would prefer a single intrinsic/opcode with additional predicates to handle signalling vs. quiet. My main reason is consistency with how the IEEE-754 spec describes comparisons, but that's a weak argument. @craig.topper mentioned to me that there are some additional opcodes that might need to be duplicated if you treat signalling and quiet as separate operations.
Nov 20 2019
Nov 19 2019
Nov 15 2019
As referenced in Simon's reply, I have started a mailing list discussion about this.
Nov 14 2019
This looks good to me. I like the header file name and location you have here.
Nov 13 2019
Nov 12 2019
Thanks. I understand your direction for denormal handling now, and I'm OK with this patch apart from the remaining references to subnormal that Sanjay mentioned.
Nov 11 2019
@pcc I've added you as a reviewer in hopes that you can tell me whether my use of code in globalCleanup() is correct. It seems to be working, but Craig was concerned that there might be cases under which this would be called when things weren't sufficiently materialized to properly handle the distinction between function declaration and definition.
Nov 8 2019
I'm unclear as to the expectations surrounding this option. I suppose this is somewhat beyond the scope of the current changes, but I'm confused by clang's current behavior with regard to denormals.
Nov 5 2019
I like this. I have just one minor suggestion.
Nov 4 2019
This looks like a very good start. I've talked to @pengfei about the x86 backend support for this. As long as x86 doesn't fail horribly, I'd be OK with the x86 work being done in a separate patch that depends on this one.
Oct 29 2019
Oct 21 2019
Thanks for the patch! I don't have time to review this in detail this week, but I'm very happy to see this functionality.
Oct 9 2019
This patch seems to be doing at least two different things. Can you separate the changes related to strict node handling into a separate patch?
Oct 3 2019
This looks good to me, but I'd like @craig.topper to verfiy that he's happy with the parts he commented on.
I assume you've tested this with your verifier changes. This is a bit too much to verify every call and function manually. I looked at the ones that weren't boilerplate updates, and I sampled the boilerplate changes. Based on that review strategy, it looks good to me.
Sep 25 2019
This looks good to me.
Sep 24 2019
It was probably me that argued for the strictfp attribute being required on the function definition. My reasoning was that when the inliner wants to inline a function containing strict FP calls into a function that does not, it would use the strictfp attribute as a cue to convert all of the FP operations in the target function to constrained. This can, of course, be deduced from the IR, but if we treat the attribute on the definition as optional then the inliner would always need to check the IR anyway. So if it isn't required it has limited usefulness.
Sep 17 2019
Need to fix Sphinx warnings
Sep 16 2019
Sep 13 2019
Thanks for updating this bit of documentation!
Sep 12 2019
Aug 27 2019
Aug 16 2019
It took some digging, but I finally found the e-mail thread where we initially agreed that we can't mix constrained FP intrinsics and non-constrained FP operations within a function. Here it is: http://lists.llvm.org/pipermail/cfe-dev/2017-August/055325.html
Aug 15 2019
Aug 14 2019
Aug 9 2019
Aug 7 2019
I'm not entirely caught up on this review. I've only read the most recent comments, but I think I've got enough context to comment on the metadata arguments.
Jul 15 2019
My understanding is that IEEE-754 requires us to produce an exception if and only if the exception would be produced by a literal interpretation of the source. However, it does not require that the exceptions be raised in the same order as implied by the source. Also, that's what the LLVM language reference says we'll do with "fpexcept.strict" -- "The number and order of floating-point exceptions is NOT guaranteed." So, I think the changes you've got here are correct.
Jul 3 2019
Jun 27 2019
I might be opening a can of worms here and I'm not a language expert, but it isn't clear to me from reading the C99 standard that defining fptosi/fptoui as returning poison values in the unrepresentable case allows correct implementation of the C standard. That is, it doesn't seem to me that the standard actually says this is undefined behavior. It just says the resulting value is unspecified, and the exception behavior is explicitly defined. On the other hand, C++ does say clearly that it is undefined behavior, right?
For the constrained intrinsics, at least in the case where the exception semantics argument is "fpexcept.strict", we need to make sure the invalid exception is raised. If the exception semantics argument is "fpexcept.ignore" or "fpexcept.maytrap" we could allow the conversion to be optimized away.
Jun 18 2019
Jun 17 2019
How would you feel about rebooting this as a new patch? There's a lot of irrelevant history here, and I feel like I'm missing some context as I review it.
Jun 4 2019
OK. I'm convinced. This should let us get a correct solution in place, and as you say we can add on something to handle rounding modes later if needed.
Jun 3 2019
Why is the FPBarrierChain getting involved with the BarrierChain for memory objects? Even without this patch I think we might be entangling the strict FP nodes with other chained nodes more than we should.