User Details
- User Since
- Jan 2 2013, 1:50 PM (419 w, 6 d)
Jun 12 2020
lgtm
Jun 9 2020
Sorry for having let this drop for so long. Some other priorities came up, but I am still interested in seeing this through.
May 19 2020
May 13 2020
May 12 2020
I don't understand why this difference exists. Beyond just trying to reproduce what MSVC does, can you explain the difference?
Apr 22 2020
Mar 30 2020
Mar 26 2020
Sorry for the delay in approval.
Mar 12 2020
OK. Since the behavior for out-of-range evl is target-dependent, undefined makes sense.
Mar 11 2020
Can you use the "Edit Related Revisions" link to set the parent/child relationships of these patches and put "[1/5]" in the titles?
What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.
I'm satisfied with the functionality, but I'm not sure about the intrinsics having undefined behavior outside the [0, W] range. The way you've implemented it, it seems like the behavior would be predictable. If the evl argument is outside that range, it is ignored. Applying an unsigned value greater than W using the "%mask AND %EVLmask" also has this effect. Why not just make that the defined behavior?
Mar 10 2020
Mar 4 2020
lgtm
Feb 27 2020
Feb 25 2020
I may have overstated. The exception behavior of the
Feb 24 2020
Any more feedback? Should I proceed in this direction?
Feb 21 2020
I don't know enough about PowerPC to review the check in the tests, but the basic constrained intrinsic handling looks correct.
It's a little weird that we're still recognizing these as LibFuncs, but I guess that's necessary to keep the optimizations like constant folding working with them. I see that ConstantFolding already calls the non-finite-restricted versions of the functions when it's doing compile-time evaluation. It would be good to have a comment somewhere (probably in TargetLibraryInfo.cpp below the note about math-finite.h) explaining the handling of these functions.
Feb 19 2020
Feb 14 2020
Feb 13 2020
Feb 12 2020
lgtm
Feb 11 2020
Feb 7 2020
lgtm
Feb 6 2020
Feb 4 2020
I feel like the bar should be fairly low for committing this. The proposal itself will notify those who are interested (agree or disagree) where they can join the discussion.
ping
OK. I was picturing MVL as some sort of maximum supported by the hardware in some sense or context. I think(?) I've got it now.
I like your first proposal for optional parameters. It "feels right" to me, and I agree that it is an improvement on operand bundles. Obviously it would take some time to build consensus for something like that as a new IR construct. I can live with the explicit parameter for now if we can agree on rules for how it is defined to behave on targets where it does not apply.
Feb 3 2020
Jan 31 2020
I know it's very late for me to be bringing this up, but I have a couple of broad questions -- one minor, one less minor.
lgtm
Jan 30 2020
Thanks for the review!
Rebase and fix typo
Jan 29 2020
I have a couple of comments, but nothing that couldn't be addressed in a later patch.
Jan 27 2020
It's not clear to me from reading this how the "precise" control is going to work with relation to the fast math flags. I don't think MSVC allows the granularity of control over these flags that we have in clang, so maybe they don't have this problem.
Jan 24 2020
Jan 23 2020
Updated to incorporate review suggestions
Jan 17 2020
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.
Jan 16 2020
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.
Jan 10 2020
Jan 9 2020
Jan 8 2020
Jan 6 2020
Jan 3 2020
lgtm
Dec 23 2019
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
lgtm
lgtm