- User Since
- Apr 14 2013, 11:48 AM (364 w, 2 d)
Fri, Apr 3
The Z ABI implementation now looks correct to me, except for one corner case: an LLVM-level argument type of f128 is passed via implicit reference. Now, in most cases this is already handled at the clang level, i.e. when you have a "long double" at C source level, you'll already see a pointer type in the LLVM IR instead. However, there are still a few cases where there is a f128 at the LLVM IR level, e.g. as arguments to some compiler builtin / libgcc routines. Those are only transformed to implicit reference in the LLVM back-end, so I believe for completeness this case should also be handled here.
Tue, Mar 31
I'm now getting an assertion failure in the LNT test suite:
Thanks for running the benchmark, I guess I'm OK with the current implementation then.
Thanks for working on this, @thakis !
Mon, Mar 30
Ah, good point. Dimitry, can you prepare an updated patch to implement Jonas' suggestion?
Fri, Mar 27
Looking simply at the SystemZ test case change, for the icmp/[zs]ext case (fun1/fun2), we actually need three instructions (compare, load zero, conditional move), so the change seems reasonable.
Thu, Mar 26
The more I think about it, the more it seems that the original check has always been somewhat questionable.
Had a look at the vararg handling. Reviewed only the ABI-relevant parts.
This doesn't apply cleanly to current mainline. Can you rebase and test again? I'll check it in then.
I'm not sure I understand those latest changes. You seem to no longer check at all whether the target opcode actually requires tied operands, you just always tie them?
Wed, Mar 25
Tue, Mar 24
See inline comments. Otherwise this looks good, but I'd rather commit this as two separate patches (the memory-immediate changes in one, and the MS(G)RKC changes in the other).
Mon, Mar 23
Oops, sorry, missed your update. Will look into that shortly.
Mon, Mar 16
Thu, Mar 12
Not yet looking at the implementation details, but a couple of comments on the overall approach:
Ah, that's a good idea. I agree this makes sense.
Tue, Mar 10
Thanks for working on this! A few comments inline.
Mar 2 2020
I see. I still agree it is preferable to only generate the overflow ops for types where they will be legal.
Feb 28 2020
Otherwise, LGTM. Thanks!
It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.
Feb 25 2020
I'm wondering if this handles all cases ... for CopyFromReg you apparently rely on the logic in EmitCopyFromReg that checks whether the value is used by some MachineNode with constrained regclass. But that logic isn't unconditionally used, e.g. it is skipped for "cloned" SUs ... not sure whether this could cause issues in more complicated scenarios.
Oh, and one more thing: either way, can you please add the original test case from D74601 so we're sure this problem is (and remains) fixed. Thanks!
Feb 23 2020
Feb 22 2020
Feb 21 2020
OK, just a few more small comments, otherwise looks really good now. Thanks!
Feb 20 2020
Yes, this now looks correct to me. However, we now have duplicated computation between LowerFormalArguments and assignCalleeSavedSpillSlots again, which I don't really like.
Feb 17 2020
Feb 13 2020
I believe the conversion of SNaN to QNaN is expected here. Note that the (current) C standard does not mention support signaling NaNs at all, and does not really ever mention them. This is planned to be fixed with the upcoming C2x version, which explicitly states that "rint" is supposed to implement the IEEE-754 "roundToIntegerExact" function. And that function, like most general functions defined by IEEE-754, is indeed defined to return a QNaN when the input is a SNaN.
Feb 11 2020
I guess it would be nicer if we could still handle this case somehow.
Feb 10 2020
Feb 7 2020
We need the "+soft-float" feature in UsesVectorABI(), so we have the front end add it, so we don't need to check "-use-soft-float"="true", like the other targets, right?
LGTM as well.
For S390, type code 'g' is used for 128-bit long double literals
Feb 4 2020
Feb 3 2020
Jan 31 2020
Jan 28 2020
174 ↗(On Diff #238345)
ok, removed it from this patch.
I had to change soft-float-02.ll to use -mattr=soft-float instead of a function attribute after removing this.
Jan 27 2020
Jan 24 2020
Jan 21 2020
Jan 20 2020
I've had a quick look at GCC, and it seems there's a couple of different issues.
Jan 17 2020
The constrained fcmp intrinsics don't allow the TRUE/FALSE predicates.
Jan 16 2020
What are the semantics of vfnmadb with respect to when it rounds vs the negation?
A few comments (see inline) -- otherwise this looks good to me, thanks!
Jan 15 2020
Why do we need to copy this algorithm here? Can't you simply add this case to TargetLowering::expandUINT_TO_FP instead?
Jan 14 2020
It turns out this patch introduced a regression with chain handling in code mixing constrained intrinsics and memory operations.
Jan 13 2020
Jan 12 2020
Looks like the remaining test case changes are strictly due to scheduling.
Address review comments and rebase.
Jan 10 2020
Updated patch now that D72224 has landed. At this point, there are no test suite failures remaining.
OK, this looks all good to me now. I agree that there can be future API cleanup, but that can be done in follow-on patches. I think we should go with this for now.
Jan 9 2020
Jan 8 2020
In general this all makes sense to me. See a few minor inline comments.
Use SmallVector::append instead of ::insert.