Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (401 w, 5 d)

Recent Activity

Today

rjmccall accepted D88664: [AST] do not error on APFloat invalidOp in default mode.

LGTM.

Thu, Oct 1, 9:04 AM

Yesterday

rjmccall added a comment to D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

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.

Wed, Sep 30, 11:59 AM · Restricted Project
rjmccall added a comment to D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

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.

Wed, Sep 30, 11:56 AM · Restricted Project

Tue, Sep 29

rjmccall abandoned D88497: [objc] Fix memory leak in CGObjCMac.cpp.
Tue, Sep 29, 4:53 PM · Restricted Project, Restricted Project
rjmccall added a comment to D88497: [objc] Fix memory leak in CGObjCMac.cpp.

Committed (with substantial changes) as 984744a1314ce165378e7945bc45995302a8cb80

Tue, Sep 29, 4:52 PM · Restricted Project, Restricted Project
rjmccall commandeered D88497: [objc] Fix memory leak in CGObjCMac.cpp.
Tue, Sep 29, 4:52 PM · Restricted Project, Restricted Project
rjmccall committed rG984744a1314c: Fix a variety of minor issues with ObjC method mangling: (authored by rjmccall).
Fix a variety of minor issues with ObjC method mangling:
Tue, Sep 29, 4:52 PM
rjmccall accepted D88518: Recognize setjmp and friends as builtins even if jmp_buf is not declared yet..

LGTM. I like this better as a fix for PR40692 anyway.

Tue, Sep 29, 3:45 PM · Restricted Project
rjmccall added a comment to D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

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.

Tue, Sep 29, 2:41 PM · Restricted Project
rjmccall added a comment to D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress.

Richard and I had a long conversation about this further up-thread, if you missed it.

Tue, Sep 29, 10:24 AM · Restricted Project
rjmccall added a comment to D88325: IR: Reject unsized sret in verifier.

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.

Tue, Sep 29, 10:19 AM · Restricted Project
rjmccall added a comment to D54749: Saturating float to int casts..

Design seems reasonable to me, but I can't review LLVM CodeGen patches.

Tue, Sep 29, 10:15 AM · Restricted Project
rjmccall added a comment to D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes.

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.

Tue, Sep 29, 10:12 AM · Restricted Project
rjmccall added a comment to rGf91b9c0f9858: Run test on particular target only.

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.

Tue, Sep 29, 10:07 AM
rjmccall added a comment to D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

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?).

Tue, Sep 29, 10:05 AM · Restricted Project
rjmccall accepted D88497: [objc] Fix memory leak in CGObjCMac.cpp.

Oh, oops, I should've caught this in review. I assume you still need a commit?

Tue, Sep 29, 9:57 AM · Restricted Project, Restricted Project
rjmccall added a comment to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.

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.

Tue, Sep 29, 12:36 AM · Restricted Project

Mon, Sep 28

rjmccall added a comment to D88329: [objc] Consolidate ObjC name mangle code to AST.

Done as 98ef7e29b0fe03da77fa6ef5c86bea9e31c178d0

Mon, Sep 28, 11:28 PM · Restricted Project
rjmccall committed rG98ef7e29b0fe: This reduces code duplication between CGObjCMac.cpp and Mangle.cpp (authored by ellis).
This reduces code duplication between CGObjCMac.cpp and Mangle.cpp
Mon, Sep 28, 11:27 PM
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

Sounds good to me, thanks!

Mon, Sep 28, 2:20 PM · Restricted Project, Restricted Project

Fri, Sep 25

rjmccall accepted D88329: [objc] Consolidate ObjC name mangle code to AST.

Thanks, LGTM.

Fri, Sep 25, 6:47 PM · Restricted Project
rjmccall added a reviewer for D88329: [objc] Consolidate ObjC name mangle code to AST: rjmccall.
Fri, Sep 25, 2:46 PM · Restricted Project
rjmccall added inline comments to D88329: [objc] Consolidate ObjC name mangle code to AST.
Fri, Sep 25, 2:45 PM · Restricted Project

Thu, Sep 24

rjmccall added inline comments to D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata.
Thu, Sep 24, 8:53 PM · Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

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.

how about seh.cppscope.begin() and seh.cppscope.end()? or any suggestion?

Thu, Sep 24, 4:41 PM · Restricted Project, Restricted Project
rjmccall added a comment to D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

Yeah, that would also be a completely reasonable approach. We already preserve the source spelling of the incomplete array type in the appropriate expression.

Thu, Sep 24, 12:20 PM
rjmccall accepted D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

Thanks, LGTM.

Thu, Sep 24, 12:07 PM
rjmccall added a comment to D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata.

Okay, the result of internal review is that we're comfortable with this feature.

Thu, Sep 24, 11:57 AM · Restricted Project
rjmccall added inline comments to D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.
Thu, Sep 24, 11:04 AM
rjmccall accepted D85961: [Fixed Point] Add floating point methods to APFixedPoint..

LGTM

Thu, Sep 24, 10:56 AM · Restricted Project
rjmccall accepted D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes.
Thu, Sep 24, 10:39 AM · Restricted Project
rjmccall added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

LGTM. Although another option would just be to change how we do this "conversion" at parse time.

Thu, Sep 24, 10:03 AM · Restricted Project
rjmccall added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

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.

Thu, Sep 24, 9:05 AM · Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

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.

Thu, Sep 24, 1:29 AM · Restricted Project, Restricted Project

Wed, Sep 23

rjmccall added inline comments to D77248: [llvm][IR] Add dso_local_equivalent Constant.
Wed, Sep 23, 10:03 PM · Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

There is absolutely NO explicit edge added in this design.

I didn't say there was. I said that there was an *implicit* edge: from the memory access to the handler. It's precisely because that edge is implicit that it's problematic for analysis and optimization.

Oh, sorry I misread it.
For HW exceptions the 'implicit' edge is unavoidable unless an explicit approach (like previous iload/istore) is employed. iload approach was extensively discussed and evaluated when it's proposed. As far as I know, the concerns were that it could create many Invokes, complicate flow graph and possibly result in negative performance impact for downstream optimization and code generation. Making all optimizations be aware of the new semantic is also substantial.

Hmm. I suppose it's not that different from the general problem with setjmp/longjmp. I think you'll still have representation problems here, but I'll address them below; I concede that in principle marking regions with an intrinsic that instructions can't necessary be moved over is workable, and so you don't need to turn every faulting instruction into an invoke. You may need to mark the initial seh.try.begin with something like returns_twice or otherwise communicate that there's non-obvious control flow within that function.

OK, good to know this. thank you!
Other than blocking some opts, does this returns_twice attribute have some other implications?

So this design applies an IMPLICT approach. I respectfully disagree that it's problematic because as long as volatile attribute is honored it's robust. please see the C & C++ SEH rule stated in patch description section.

You also need to make sure that potentially-faulting instructions aren't moved *into* the _try region. I don't think that just making accesses with the _try volatile achieves that. Probably the easiest way to do this would be to outline the _try region through most of the LLVM pipeline and only inline it very late.

The intrinsic is set with unknown memory access. Plus returns_twice you just suggested, is not it sufficient to block potentially-faulting instructions from being moved across?

Wed, Sep 23, 9:20 PM · Restricted Project, Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

@rjmccall wrote:
I think you're missing what I'm asking. If LLVM accepts this feature, it will become our collective responsibility as a project to keep it working. You have a large external correctness test suite for this feature. It does not sound like you intend to open-source that test suite; instead, you intend to extract a small number of unit tests from it and add those to the LLVM test suite. So I'm asking if you're at least going to have an external CI bot which will run the full test suite for this feature to ensure it hasn't been broken by the last day of commits. It does not seem reasonable to expect that the few unit tests you extract will themselves be sufficient to keep the feature tested.

Microsoft's SEH tests are open source and there was a suggestion in one of the threads on llvm-dev to run them on the buildbots. We can look into that as a follow on to this patch.

https://lists.llvm.org/pipermail/llvm-dev/2019-November/136695.html
https://lists.llvm.org/pipermail/llvm-dev/2020-April/140614.html

Wed, Sep 23, 5:50 PM · Restricted Project, Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

There is absolutely NO explicit edge added in this design.

I didn't say there was. I said that there was an *implicit* edge: from the memory access to the handler. It's precisely because that edge is implicit that it's problematic for analysis and optimization.

Oh, sorry I misread it.
For HW exceptions the 'implicit' edge is unavoidable unless an explicit approach (like previous iload/istore) is employed. iload approach was extensively discussed and evaluated when it's proposed. As far as I know, the concerns were that it could create many Invokes, complicate flow graph and possibly result in negative performance impact for downstream optimization and code generation. Making all optimizations be aware of the new semantic is also substantial.

Wed, Sep 23, 2:59 PM · Restricted Project, Restricted Project
rjmccall added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

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.

Wed, Sep 23, 11:04 AM · Restricted Project

Tue, Sep 22

rjmccall added a comment to D85961: [Fixed Point] Add floating point methods to APFixedPoint..

If you disagree, I'm not going to insist, though.

Tue, Sep 22, 10:39 AM · Restricted Project
rjmccall added inline comments to D85961: [Fixed Point] Add floating point methods to APFixedPoint..
Tue, Sep 22, 10:38 AM · Restricted Project

Mon, Sep 21

rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

Thank you for prompt reply again.

[rjmccall] And I agree with him that the potential benefits are substantial. I'm just saying that one barely-trafficked RFC thread is not evidence of a community consensus.

OK, thanks. it's good to know you are also supportive in adding this feature.

[rjmccall] As I understand it, you are creating implicit edges within the function to the active SEH handler (potentially part of the same function) from arbitrary places within covered basic blocks, turning basic blocks from a single-entry single-(internal-)exit representation to a single-entry multiple-exit representation. That is a huge change. How do you expect LLVM transformations to preserve or merge this information across blocks? How do you expect transformations to not move code between blocks in problematic ways, or reorder code within blocks in ways that will suddenly be visible to the optimizer? These are the standard objections to supporting features like this, that they have basically unspecified behavior and appear to work by good intentions and happy thoughts.

There is absolutely NO explicit edge added in this design.

Mon, Sep 21, 9:35 PM · Restricted Project, Restricted Project
rjmccall added a comment to D86632: [Fixed Point] Add codegen for conversion between fixed-point and floating point..

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.

Mon, Sep 21, 9:12 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D85961: [Fixed Point] Add floating point methods to APFixedPoint..
Mon, Sep 21, 10:11 AM · Restricted Project

Sun, Sep 20

rjmccall accepted D87983: [Sema] Split special builtin type lookups into a separate function.

Thanks. Minor improvement and then LGTM.

Sun, Sep 20, 2:50 PM · Restricted Project

Fri, Sep 18

rjmccall added inline comments to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).
Fri, Sep 18, 11:26 AM · Restricted Project
rjmccall added a comment to D87917: [Sema] Handle objc_super special lookup when checking builtin compatibility.

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.

Fri, Sep 18, 11:17 AM · Restricted Project
rjmccall added inline comments to D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes.
Fri, Sep 18, 11:13 AM · Restricted Project
rjmccall added inline comments to D87917: [Sema] Handle objc_super special lookup when checking builtin compatibility.
Fri, Sep 18, 11:05 AM · Restricted Project

Thu, Sep 17

rjmccall added inline comments to D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes.
Thu, Sep 17, 10:56 PM · Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

Hi, John, thank you for reviewing this patch and providing feedback.
regarding your comments:

1, In the RFC thread early, Reid K (the major contributor of Windows SEH/EH support) had agreed that "the value proposition is clear and large".

Thu, Sep 17, 9:11 PM · Restricted Project, Restricted Project
rjmccall added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

If we started with a qNaN, then we can't hit the bug, right? Ie, if the MSB of the significand is set, there's no way we could shift that out. If that's correct, then we could just always set the bottom bit? That way sNaN remains sNaN even if lose the payload.

Or maybe that's not true in the case of the x87 or other special formats?

Thu, Sep 17, 8:51 PM · Restricted Project
rjmccall added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

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.

Thu, Sep 17, 11:38 AM · Restricted Project

Wed, Sep 16

rjmccall accepted D87761: [clang][codegen] Skip adding default function attributes on intrinsics..

Seems reasonable.

Wed, Sep 16, 10:15 AM · Restricted Project

Tue, Sep 15

rjmccall accepted D87731: [Coro][NewPM] Handle llvm.coro.prepare.retcon in NPM coro-split pass.

Thanks, this looks right.

Tue, Sep 15, 8:45 PM · Restricted Project

Mon, Sep 14

rjmccall updated subscribers of D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress.

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?

Mon, Sep 14, 4:09 PM · Restricted Project

Sun, Sep 13

rjmccall added a comment to D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress.

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:

Sun, Sep 13, 11:55 PM · Restricted Project

Fri, Sep 11

rjmccall accepted D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle.

Thanks, LGTM.

Fri, Sep 11, 1:31 PM · Restricted Project
rjmccall added a comment to D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress.

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.

Fri, Sep 11, 12:03 PM · Restricted Project
rjmccall added a comment to D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle.

hmm @rjmccall, I don't think there is a stable way to test this. The code generated for symmetric transfer is way too complicated to stably pattern match one less item in the frame.

Fri, Sep 11, 11:50 AM · Restricted Project
rjmccall accepted D85473: [Clang] Add option to allow marking pass-by-value args as noalias..

Just a minor tweak and then LGTM.

Fri, Sep 11, 11:46 AM · Restricted Project
rjmccall accepted D87512: [IR] Add Type::getFloatingPointTy..

LGTM, thanks!

Fri, Sep 11, 10:53 AM · Restricted Project

Thu, Sep 10

rjmccall added inline comments to D86632: [Fixed Point] Add codegen for conversion between fixed-point and floating point..
Thu, Sep 10, 10:26 PM · Restricted Project, Restricted Project
rjmccall added a reviewer for D85473: [Clang] Add option to allow marking pass-by-value args as noalias.: ahatanak.
Thu, Sep 10, 10:16 PM · Restricted Project
rjmccall added a comment to D85960: [AST][FPEnv] Keep FP options in trailing storage of CastExpr.

Follow-up on my previous request. It's fine by me if you commit with that fix.

Thu, Sep 10, 10:12 PM · Restricted Project
rjmccall added a comment to D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle.

Also LGTM with a testcase. It's fine to test the result of IRGen.

Thu, Sep 10, 10:10 PM · Restricted Project
rjmccall added a comment to D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata.

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.

But if it's just a forward declaration there's nothing to emit. The above code compiles just fine as is with just a warning. Here's the result of clang protocol.m -lobjc

proto.m:10:31: warning: cannot find protocol definition for 'NonRuntimeProto'
@interface Implementer : Root<NonRuntimeProto>
                              ^
proto.m:8:11: note: protocol 'NonRuntimeProto' has no definition
@protocol NonRuntimeProto;
          ^
1 warning generated.
Undefined symbols for architecture x86_64:
  "__OBJC_PROTOCOL_$_NonRuntimeProto", referenced from:
      __OBJC_CLASS_PROTOCOLS_$_Implementer in proto-bd4a43.o
ld: symbol(s) not found for architecture x86_64
clang-12: error: linker command failed with exit code 1 (use -v to see invocation)

The protocol definition isn't actually required to compile an implementation. And if that protocol is declared as objc_non_runtime_protocol it won't ever see one.

Simply requiring that it is annotated accordingly also isn't satisfactory for the same inheritance problem you mentioned above. Annotating a forward decl can tell clang not to emit it but won't let clang know if there's a base protocol that still needs to be emitted. e.g. if we have

// SomeHeader.h
@protocol Base
@end
__attribute__((objc_non_runtime_protocol))
@protocol NonRuntime <Base>
@end


// and in main.m
__attribute__((objc_non_runtime_protocol))
@protocol NonRuntime
@interface AClass : Root<NonRunime>
@end
@implementation AClass
@end

we'll get a compile warning but no link error. But it will be wrong as the protocol Base should still be in the protocollist for AClass.

I'm not sure how big of an issue it would be introducing a new error here for code that used to compile, but that's really the only way I see this working.

Thu, Sep 10, 10:07 PM · Restricted Project

Wed, Sep 9

rjmccall added a comment to D87331: Sema: add support for `__attribute__((__swift_error__))`.

const qualification isn't the most meaningful thing for Objective-C objects anyway.

Wed, Sep 9, 1:07 PM · Restricted Project
rjmccall added inline comments to D87331: Sema: add support for `__attribute__((__swift_error__))`.
Wed, Sep 9, 11:06 AM · Restricted Project

Tue, Sep 8

rjmccall accepted D87331: Sema: add support for `__attribute__((__swift_error__))`.

LGTM, thanks!

Tue, Sep 8, 6:18 PM · Restricted Project
rjmccall added a comment to D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata.

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.

Tue, Sep 8, 6:15 PM · Restricted Project
rjmccall added a comment to D87331: Sema: add support for `__attribute__((__swift_error__))`.

Oh, there are a ton of typos in this documentation.

Tue, Sep 8, 3:35 PM · Restricted Project
rjmccall accepted D87331: Sema: add support for `__attribute__((__swift_error__))`.

LGTM. :)

Tue, Sep 8, 2:51 PM · Restricted Project

Mon, Sep 7

rjmccall added a comment to D85960: [AST][FPEnv] Keep FP options in trailing storage of CastExpr.

Minor request, but otherwise LGTM.

Mon, Sep 7, 11:41 AM · Restricted Project
rjmccall added inline comments to D85961: [Fixed Point] Add floating point methods to APFixedPoint..
Mon, Sep 7, 11:36 AM · Restricted Project

Fri, Sep 4

rjmccall added a comment to D85961: [Fixed Point] Add floating point methods to APFixedPoint..

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.

Fri, Sep 4, 11:06 AM · Restricted Project

Thu, Sep 3

rjmccall added a comment to D85473: [Clang] Add option to allow marking pass-by-value args as noalias..

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?

Thu, Sep 3, 10:36 PM · Restricted Project
rjmccall added a comment to D85961: [Fixed Point] Add floating point methods to APFixedPoint..

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?

Thu, Sep 3, 10:30 PM · Restricted Project
rjmccall accepted D86921: [FPEnv] Partially implement #pragma STDC FENV_ROUND.

LGTM

Thu, Sep 3, 10:26 PM · Restricted Project
rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

I've made some brief comments about the code, but I think I have much larger concerns here.

Thu, Sep 3, 9:42 PM · Restricted Project, Restricted Project
rjmccall added a comment to D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods..

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.

Thu, Sep 3, 9:11 PM · Restricted Project
rjmccall accepted D87102: [Sema] Fix a -Warc-repeated-use-of-weak false-positive by only calling CheckPlaceholderExpr once.

LGTM

Thu, Sep 3, 1:16 PM · Restricted Project
rjmccall updated subscribers of D86993: Document Clang's expectations of the C standard library..

@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?).

Thu, Sep 3, 10:52 AM · Restricted Project
rjmccall added a comment to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.

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?

Thu, Sep 3, 10:39 AM · Restricted Project
rjmccall added a comment to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.

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.

Thu, Sep 3, 12:49 AM · Restricted Project

Tue, Sep 1

rjmccall added a comment to D86993: Document Clang's expectations of the C standard library..

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?

Tue, Sep 1, 6:10 PM · Restricted Project
rjmccall accepted D86980: [Docs] Remove outdated OS limitation.

Thanks!

Tue, Sep 1, 2:42 PM · Restricted Project
rjmccall added a comment to D86815: [LangRef] Adjust guarantee for llvm.memcpy to also allow equal arguments..

It should be separate, yeah.

Tue, Sep 1, 1:57 PM · Restricted Project
rjmccall added a comment to D86815: [LangRef] Adjust guarantee for llvm.memcpy to also allow equal arguments..

There is a longstanding assumption made by ~every compiler that memcpy(p, p, n) is safe. That's what we should be encoding here. We should not be removing all overlap restrictions.

If clang doesn't have a document stating the assumptions made about the run-time libraries (couldn't quickly find it), it might be useful to have one and mention this.

That sounds good, but I am not sure where this should be documented on the Clang side. I think this can also be done separately.

Tue, Sep 1, 12:22 PM · Restricted Project

Aug 31 2020

rjmccall added inline comments to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.
Aug 31 2020, 2:43 PM · Restricted Project

Aug 30 2020

rjmccall accepted D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context.

LGTM modulo the minor fix

Aug 30 2020, 10:58 PM · Restricted Project
rjmccall added a comment to D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness.

I'd still like @rsmith to sign off here as code owner.

Aug 30 2020, 10:54 PM · Restricted Project
rjmccall added a comment to D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem..

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 30 2020, 5:10 PM · Restricted Project

Aug 27 2020

rjmccall added a comment to D54749: Saturating float to int casts..

At least for the IEEE formats, you should be able to just destructure the bit-pattern of the float, right? Normalize and do some extends and shifts.

If we're going down that path, maybe simpler to just mess with the float scaling factor. If a float is close to the largest finite float, in IEEE formats, the fractional part must be zero. So we can do the float-to-int conversion on the unscaled float, and scale the resulting integer. Probably faster than trying to explicitly destructure a float.

Aug 27 2020, 1:14 PM · Restricted Project
rjmccall added a comment to D54749: Saturating float to int casts..

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 27 2020, 10:53 AM · Restricted Project
rjmccall added inline comments to D85961: [Fixed Point] Add floating point methods to APFixedPoint..
Aug 27 2020, 1:33 AM · Restricted Project
rjmccall added inline comments to D86668: Fix Calling Convention of __float128 and long double(128bits) in i386.
Aug 27 2020, 1:11 AM · Restricted Project

Aug 26 2020

rjmccall added a comment to D86354: [ADT] Make Optional a literal type..

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.

Aug 26 2020, 7:29 PM · Restricted Project
rjmccall added a comment to D86354: [ADT] Make Optional a literal type..

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.

Aug 26 2020, 12:44 PM · Restricted Project
rjmccall accepted D86354: [ADT] Make Optional a literal type..

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.)

Aug 26 2020, 12:19 PM · Restricted Project