- User Since
- Jan 2 2013, 1:50 PM (306 w, 1 d)
Tue, Nov 13
Thu, Nov 8
Agreed. Reverting this patch wouldn't move us forward on constrained FP handling. What I'm saying (and what I think @nastafev is saying) is that this patch is taking a built-in that allows the user to express specific signalling behavior and throwing away that aspect of its semantics. Granted we do say that FP environment access is unsupported, so technically unmasking FP exceptions or even reading the status flag is undefined behavior. But it seems like there's a pretty big difference between using that claim to justify enabling some optimization that might do constant folding in a way that assumes the default rounding mode versus using that claim to disregard part of the semantics of a built-in that is modeling a target-specific instruction.
Craig Topper also raised some concerns with me (in person, his desk is right by mine) about the potential effect this might have on code size. He tells me that IRBuilder calls are intended to always be inlined and if we grow the implementation of these functions too much it could lead to noticeable bloat. It still seems to me like it might be worthwhile for the simplification it would allow in the front end, but I'm not really a front end guy so I definitely agree that we should get some input from front end people about what they want.
Wed, Nov 7
I think my preference would be to have the predicate in the function name. I briefly toyed with the idea of hacking AsmWriter to print a constant integer as the corresponding predicate string, but I think that would look too weird. Also, I don't think we should open the door to something trying to use the value that represents the predicate as if it were a real value. In this sense the predicate argument, if we had one, should really be a token but I don't think we want to add new token constants just for this.
Rather than having separate methods to create the constrained versions of the intrinsics, why not have a way to set the constrained state in the IR builder and have the regular CreateFAdd et. al. functions use that to automatically create the constrained intrinsic versions? This would correspond to how fast math flags are handled.
Mon, Nov 5
This is definitely a tricky case. I don't really like any of the available solutions. I'll try to think more about it, and maybe someone else will have a brilliant suggestion.
Fri, Nov 2
This looks good, except that I assume now you'll be removing the FMA checks from the tests.
Thu, Nov 1
Thu, Oct 25
Wed, Oct 24
Oct 9 2018
Oct 1 2018
Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?
Sep 25 2018
Sep 18 2018
Aug 23 2018
Aug 21 2018
I feel like it might be a bad idea to have floating point instructions that don't have constrained forms.
Aug 2 2018
I hope I'm not coming across as too argumentative here. I don't really have strong feelings about this function pointer type patch and ultimately I see that you are right, but the objections you are raising here would equally apply to what I'm working on with the IR linker and if I find a way to fix that, I'll care a bit more about that case. (If you'd like a preview, here's the bug I'm trying to fix: https://bugs.llvm.org/show_bug.cgi?id=38408)
Aug 1 2018
The LLVM linker also seems to have a bunch of problems with resolving pointer types in structures. I'm looking into a couple of those now.
I've talked to Olga about this, and I think we have a way to handle the problem she was working on without this change.
Jun 14 2018
Jun 13 2018
Jun 11 2018
Jun 8 2018
Thanks for the patch!
Jun 7 2018
I'm adding Craig Topper as a reviewer because he knows the vector selection DAG stuff better than I do. The constrained FP handling looks OK to me.
May 21 2018
The reason that I originally implemented this the way I did, mutating the strict nodes to their non-constrained equivalents, was that I thought it would require too much duplication in the .td files to implement all the pattern matching for the strict nodes. The original plan was to find some other way to communicate the "strict" state to the target after instruction selection, but I never found a way to do that.
May 18 2018
At some point we should create a document that describes the entire flow of FP instructions through the instruction selection process. To be honest I don't remember how it all works, and that makes it difficult to review changes like this. It would also be nice to verify that we all have the same understanding of how it works. I don't mean to volunteer you to produce the entire document, but would you mind giving me a rough outline? I'm still concerned about the case that is not chained.
Apr 23 2018
Apr 4 2018
Thank you for your patience in addressing my concerns.
Apr 3 2018
Mar 27 2018
Mar 26 2018
I'm OK with this.
Mar 23 2018
Mar 22 2018
Mar 20 2018
As long as you're OK with the fact that this will probably be replaced in the near future (and please add a comment indicating that above the setOptPassGate function), I'm OK with this being added. I do prefer OptPassGate as a name for the generalized functionality.
Mar 19 2018
Mar 15 2018
Mar 14 2018
I have some concerns about this patch, mostly centered around what the intended use cases are.
Mar 7 2018
Thanks for taking the initiative on this!
Mar 6 2018
Feb 28 2018
Feb 21 2018
Feb 20 2018
Feb 9 2018
Jan 30 2018
Jan 25 2018
BTW, thank you very much for this patch!
Jan 4 2018
After thinking through this particular situation a bit more with regard to the STRICT_EXP node, I think what you've chosen to do here is probably correct. I'm not entirely certain what the exp_finite implementation does, but I would expect that with regard to rounding it will produce the same result as the normal function as long as the input is finite. Similarly, I think that the exception behavior of exp_finite should be the same as the non-finite version as long as the input is finite. If the input is non-finite, then I would expect that the appropriate exception was raised or status flag set by whatever produced the value. I don't think either exp or exp_finite will produce an exception for non-finite values. We'll get the wrong answer with exp_finite of course, but the user signed up for that when using the fast math flags.
Jan 3 2018
Jan 2 2018
Dec 11 2017
Nov 29 2017
Death to the curlies!
Nov 28 2017
Updated the test case.
Nov 27 2017
-Refactored getSuperRegDestIfDead() to consolidate the liveness checking logic.
-Added a test case.
Oct 26 2017
Oct 18 2017
It isn't clear to me how your custom lowering interacts, if at all, with existing table-driven selection patterns. One of the goals in the implementation up to this point has been to have the instruction selection fall back on existing pattern matching as much as possible so that we don't need to duplicate all of the cases that are currently handled. Can you explain to me how this applies in the AMDGPU case?
Oct 6 2017
Sep 20 2017
This was committed as r313784. I put the wrong differential revision number in the comment for that check-in.
Sep 19 2017
This is breaking buildbots for 32-bit targets because the offset in 'nullptr + int8_t_N' is being implicitly cast to an int. That makes the sizeof(offset) == sizeof(ptr) check turn out differently than my tests were assuming.
Sep 14 2017
-Changed GNU idiom from extension to warning.
-Updated to ToT.
Sep 13 2017
Does anything else need to be done for this to be ready to land?
Sep 5 2017
Aug 30 2017
Fixed value-dependent argument in isNullPointerConstant checks.
Added check for C++ zero offset in subtraction.
Added value-dependent test cases.
Refactored the GNU idiom check to be shared by Sema and CodeGen.
Refined the checks so that nullptr+0 doesn't warn in C++.
Added the zero offset qualification in the warning produced with C++.
Aug 29 2017
Added warnings for null pointer arithmetic.
Aug 25 2017
Aug 23 2017
My intention here was to make this as strict/limited as possible while still handling the cases of interest. There are two different implementations I want to handle. You can see the first variation in the __BPTR_ALIGN definition being added here:
Can you revert the white space changes in the places you aren't otherwise modifying? In general, you shouldn't make formatting changes outside of the parts of the file your patch is modifying. It complicates the version control blame process without adding a lot of benefit.