- User Since
- Jan 18 2013, 11:30 AM (401 w, 5 d)
Hmm. Is there a way to actually get an sNaN in floating-point type? I'm concerned that we might be pushing a formal argument in a way that undermines usability.
Tue, Sep 29
Committed (with substantial changes) as 984744a1314ce165378e7945bc45995302a8cb80
LGTM. I like this better as a fix for PR40692 anyway.
I think we can split it into a separate patch even if we have no way to test it yet. It's a significantly broader change, at least in theory.
Richard and I had a long conversation about this further up-thread, if you missed it.
sret marks that a parameter needs the special indirect-return argument ABI, which does *not* require a sized type on many architectures. Swift actually relies on being able to trigger this ABI on a value of unknown size. Swift can, of course, just use a meaningless sized type, but since I assume you intend to actually use the type for something, I am concerned.
Design seems reasonable to me, but I can't review LLVM CodeGen patches.
Commented in that review, but that patch has the wrong fix: it's based on the targets LLVM is configured with rather than the current test target.
The REQUIRES line is wrong; this needs to be based on whether the tested target is x86, not whether LLVM has been built with x86 support.
The change to ExprConstant should be a different patch, with a test that we can still constant-evaluate something else that results in opInvalid (I assume there must be something?).
Oh, oops, I should've caught this in review. I assume you still need a commit?
Being permissive about recognizing builtins when the expected signature requires a type that lookup can't find seems completely reasonable. We don't really want to force library functions to take the custom-typechecking path just because we want to infer an attribute for them.
Mon, Sep 28
Sounds good to me, thanks!
Fri, Sep 25
Thu, Sep 24
Yeah, that would also be a completely reasonable approach. We already preserve the source spelling of the incomplete array type in the appropriate expression.
Okay, the result of internal review is that we're comfortable with this feature.
LGTM. Although another option would just be to change how we do this "conversion" at parse time.
Okay. That honestly seems like something that should just be fixed — it's easy to sniff the literal length to figure out which format we have. But it's probably reasonable to preserve sNaNs in the short term and then come back to this.
Okay. I think we're on the same page about representation now. If you can come up with a good replacement for "eha" in the intrinsic names, I think this is pretty much ready to go.
Wed, Sep 23
Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.
Tue, Sep 22
If you disagree, I'm not going to insist, though.
Mon, Sep 21
I had a question in the other patch about whether you should just have a method on FixedPointSemantics that returns the unscaled semantics (since FixedPointSemantics is totally capable of expressing integer types), which would let fitsInFloatSemantics have more obvious semantics. That would affect this as well. But otherwise I have no concerns.
Sun, Sep 20
Thanks. Minor improvement and then LGTM.
Fri, Sep 18
Oh, I didn't notice that this only did work conditionally based on the builtin ID. Yes, please make a function like lookupNecessaryTypesForBuiltin that takes the builtin ID. Maybe we can generalize this to solve the problem with that other builtin, too.
Thu, Sep 17
Well, I went and did a whole investigation here, only to come around to the idea that what you're doing might be the most reasonable thing.
Wed, Sep 16
Tue, Sep 15
Thanks, this looks right.
Mon, Sep 14
That's a really useful concept, and given that it exists, I agree that we shouldn't invent something else. The fact that it covers local variables with constant initializers that happen to be const seems really unfortunate, though. @scanon, any opinions here about how important it is to give consistent semantics to a floating-point expression in a static local initializer in C and C++ under #pragma STDC FENV_ACCESS?
Sun, Sep 13
Richard, what do you think the right rule is for when to use the special constant-evaluation rules for this feature in C++? The C standard says that constant expressions should use the default rules rather than considering the dynamic environment, but C can get away with that because constant expressions are restricted to a narrow set of syntactic situations. In C++, [cfenv.syn]p1 says:
Fri, Sep 11
IRGen generally doesn't query the AST constant evaluator for arbitrary expressions; we do it for certain calls and loads because otherwise we might emit an illegal ODR-use, but otherwise we just expect LLVM's constant-folding to do a decent job.
Just a minor tweak and then LGTM.
Thu, Sep 10
Follow-up on my previous request. It's fine by me if you commit with that fix.
Also LGTM with a testcase. It's fine to test the result of IRGen.
Wed, Sep 9
const qualification isn't the most meaningful thing for Objective-C objects anyway.
Tue, Sep 8
I don't think it'll actually error out at link time: protocol objects get emitted eagerly on use, cross-module linking is just a code-size optimization. This actually has caused longstanding problems.
Oh, there are a ton of typos in this documentation.
Mon, Sep 7
Minor request, but otherwise LGTM.
Fri, Sep 4
It is probably reasonable to assume that there's always a type you can safely extend to such that the conversion is safe; it's very unlikely that someone would have a fixed-point type large enough to cause problems for double.
Thu, Sep 3
Is it acceptable to leave this as a -cc1 option while we're pursuing this with the language committee? Do we have any intent to pursue this with the language committee?
Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?
I've made some brief comments about the code, but I think I have much larger concerns here.
We just talk about it. I agree with Nathan that we shouldn't just add this as a short-term hack; we should design the ABI right and then do what we want.
@t.p.northover says it's complicated. memcpy, memmove, memset, and bzero are (I think) the only ones that LLVM will potentially synthesize from nothing and therefore need to be present even in freestanding builds; that's probably okay for us to guarantee. That's probably also a good place to document the supported way to write those functions in libraries (just -fno-builtin, IIRC?).
I didn't see the specific example, sorry. I agree that my description is more applicable to builtins in the __builtin namespace, which describes most of the builtins with custom typechecking. Is the problem just __va_start?
The builtins with custom type-checking are all true intrinsics like __builtin_operator_new and so on. They really can't be validly declared by the user program. The thing that seems most likely to avoid random compiler crashes would be to either forbid explicit declarations of them or treat those as no longer being builtins. If we need to maintain compatibility with people making custom declarations, we would need to always treat them as builtins and run the risk of crashing if someone declares one with a bad signature. But I don't think it's unfair of us to break those people; that is seriously not reasonable user behavior.
Tue, Sep 1
Wording looks good. Should we alsso document our assumptions about what functions exist in the standard library — the functions that we'll always use even in freestanding builds?
It should be separate, yeah.
Aug 31 2020
Aug 30 2020
LGTM modulo the minor fix
I'd still like @rsmith to sign off here as code owner.
Yes, we at Apple had similar problems and basically had to revert this internally. The basic problem is that LLVM's alloca representation is not well-suited for coroutines — we really need a guarantee that there are no stack allocations live across the coro.begin, or at least none that stay alive until the first suspend. That would be easy with something like SIL's alloc_stack, but LLVM's desire to push local allocations into the entry block really makes it difficult to even talk about the appropriate restrictions here.
Aug 27 2020
I was originally planning on adding such an intrinsic (with a scaling factor) but dropped the idea when I found this patch. You are right in that the small exponent of half precision is problematic, though. Not sure what to do about that.
Aug 26 2020
I wasn't really asking for massive constexpr-ification, just on the other constructors, but I have no problem with it given that it's done.
It's an abstractly reasonable change. I wouldn't worry about some sort of conjectured impact on build times in the absence of specific evidence.
This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr? (Note that it's okay for a templated constexpr function to instantiate to something that does stuff that's not constexpr, so this wouldn't be a major new restriction on the constructor.)