- User Since
- Jan 16 2018, 6:47 AM (47 w, 4 d)
Fri, Dec 14
Maybe a test with scale == bitwidth? Other than that it looks good to me.
Wed, Dec 12
Fix the build failures (caused by default argument promotion of float vectors).
Hmm, sorry about that. I'll have a look.
Tue, Dec 11
I don't have commit access, so I would appreciate it if you could commit the change.
Fri, Dec 7
Wed, Dec 5
Tue, Dec 4
Added testing for C++ and different sizes of int and long.
Mon, Dec 3
Fri, Nov 30
Rebased and addressed review comments.
Thu, Nov 29
Tue, Nov 27
Thu, Nov 22
Wed, Nov 21
Tue, Nov 20
Fri, Nov 16
Nov 15 2018
I'd be interested in seeing tests for two saturating unsigned _Fract with and without padding.
Nov 14 2018
The only thing I didn't include in this patch were the suggested changes to FixedPointSemantics which would indicate if the semantics were original from an integer type. I'm not sure how necessary this is because AFAIK the only time we would need to care if the semantics came from an int is during conversion from a fixed point type to an int, which should only occur during casting and not binary operations. In that sense, I don't think this info needs to be something bound to the semantics, but more to the conversion operation. I also don't think that would be relevant for this patch since all integers get converted to fixed point types anyway and not the other way around.
Nov 13 2018
Nov 6 2018
Nov 5 2018
Nov 1 2018
Oct 31 2018
Oct 30 2018
Oct 29 2018
Oct 26 2018
I think I've already expressed that I'm not at all a fan of encoding the full-precision logic in the operations themselves and not explicitly in the AST. We already have (well, not yet, but we will) routines for emitting conversions to/from/between fixed-point types of arbitrary semantics - why not use that instead of reimplementing the same logic for every binary operation?
Oct 22 2018
Oct 17 2018
Oct 16 2018
Oct 15 2018
Oct 12 2018
Looks good to me!
Oct 11 2018
Oct 10 2018
I agree with John, after that I think it's fine for me.
Oct 9 2018
Here is a module with the function right after expansion. This is generated for x86 and not our target so both this IR and the result will probably be slightly different than what I posted.
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu"
Oct 8 2018
I've been experimenting a bit with early expansion of our sat intrinsic to see how the code generation is affected by it. In general, it doesn't actually seem like there's much of an effect. In fact, neither our benchmarks nor the user code I'm looking at seem to be terribly affected by expanding the intrinsic. This is probably due to most of the instances of saturation being 'locked down' by surrounding intrinsics.
Oct 3 2018
Sep 28 2018
I think this looks pretty good to me now. It's probably a good time to add someone who is familiar with DAG as reviewer at this point.
Sep 27 2018
Sep 26 2018
Sep 25 2018
Sep 24 2018
Oh, sorry. I should have mentioned that. That would be nice, thanks!
Sep 21 2018
So if I understand this correctly, we can handle this intrinsic in 2 ways:
- The intrinsic gets converted to IR in a pass before touching the DAG.
- The intrinsic gets passed to the DAG and gets legalized and lowered to machine code there.
It seems that from the thread, the consensus was for going with (2), but I also see now how things would be much easier just having this run through a pass in (1). If I recall correctly, the main reason for offsetting the intrinsic "expansion" to the DAG was to determine if the intrinsic was supported by the hardware during the legalization steps. If so, couldn't this also be done in a pass when converting the intrinsic to IR in (1)? That is, we determine early in the pass if this intrinsic call is supported by this target. Those that aren't legal get expanded into IR whereas those that are legal get passed to DAG instruction selection.
I think this might avoid the trouble of having to check types and operations in the DAG, and ISel can choose whatever appropriate instructions for whatever supported intrinsic (if that makes sense).
Sep 20 2018
Another ping. Anyone up for reviewing this patch?
I think we need to decide where these intrinsics should be 'lowered' if they aren't legal, and define how to let the target specify legality.
Sep 7 2018
Aug 30 2018
I don't have commit access, so it would be appreciated if someone could commit it.
Added Embedded-C clause quote.
Aug 29 2018
Aug 24 2018
Aug 20 2018
I guess that's also possible. I figured it would be clearer to have the generic logic in the more obvious location (CGExprScalar) and fall back to the custom handling if needed. Many of the handlers in TargetCodeGenInfo are empty.
Aug 17 2018
Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.
Aug 16 2018
I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.
Aug 7 2018
I think the solution to a lot of diagnostic and behavior issues with address spaces is to remove predication on OpenCL. It's a bit odd to have a language feature that is enabled out of the box regardless of langoptions (address spaces) but won't actually work properly unless you're building OpenCL.
Aug 6 2018
When I try the test case on our downstream (and when compiling for our target with an extra flag that enables a bunch of OpenCL-related address space code), I get:
ascrash.c:5:12: error: comparison between ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces return x < y ? x : y; ~ ^ ~ ascrash.c:5:16: error: conditional operator with the second and third operands of type ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces return x < y ? x : y; ^ ~ ~
A lot of address space code is hidden behind LangOptions.OpenCL.
Sorry for the late response, I've been away.
Jul 4 2018
Jul 3 2018
Jun 29 2018
I also think it would be good with some unit tests for this class once the functionality and interface is nailed down.
Jun 28 2018
Jun 27 2018
Jun 26 2018
Jun 25 2018
Would it be possible to add some form of target hook (perhaps to CodeGenABIInfo, which is already accessed with getTargetHooks) for fixed-point operations (maybe even some conversions)? As I've mentioned earlier, we emit both IR and intrinsics for many of these operations to make instruction selection possible. I suspect that any target that wants to utilize fixed-point effectively will need this as well.
Jun 20 2018
Thanks! I do not have commit access, so it would be great if someone could commit this.
Jun 19 2018
Just a couple more comments and then I think it looks good.
Jun 18 2018
Jun 14 2018
Jun 13 2018
LGTM, but I'm not a code owner so maybe someone else should chime in if they have any input.