rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (304 w, 2 d)

Recent Activity

Yesterday

rjmccall added a comment to D54676: [AST] Pack CallExpr.

I don't think we should be reducing the number of call arguments we can support, sorry, even if 16K is a fairly absurd number that would probably trip stack overflow protections if you actually executed it. Let's try to keep it at least 64K-ish.

Sun, Nov 18, 11:52 PM · Restricted Project
rjmccall accepted D54675: [AST] Store the expressions in ParenListExpr in a trailing array.

LGTM.

Sun, Nov 18, 3:09 PM · Restricted Project

Fri, Nov 16

rjmccall added a reviewer for D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)": ahatanak.
Fri, Nov 16, 11:28 PM · Restricted Project
rjmccall added inline comments to D54379: Add Hurd toolchain support to Clang.
Fri, Nov 16, 7:44 PM
rjmccall added inline comments to D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang.
Fri, Nov 16, 5:04 PM
rjmccall added inline comments to D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Fri, Nov 16, 1:02 PM
rjmccall accepted D54634: [OpenCL] Fix address space deduction in template args.

Makes sense.

Fri, Nov 16, 11:46 AM
rjmccall added inline comments to D54540: [ADT] Drop llvm::Optional clang-specific optmization for trivially copyable types.
Fri, Nov 16, 10:53 AM
rjmccall added a comment to D54540: [ADT] Drop llvm::Optional clang-specific optmization for trivially copyable types.

Hmm. This makes Optional<int> non-trivial, which is a serious semantic problem for certain uses of the type (e.g. putting it in a union); it's not just an optimization. Obviously those were unportable because of the #if, but we should really fix this. There's no way that GCC just generically miscompiles all partial specializations.

Fri, Nov 16, 10:34 AM

Thu, Nov 15

rjmccall added inline comments to D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang.
Thu, Nov 15, 3:09 PM
rjmccall added inline comments to D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Thu, Nov 15, 1:01 PM
rjmccall accepted D53764: [OpenCL] Enable address spaces for references in C++ .

Thanks, LGTM.

Thu, Nov 15, 12:58 PM

Wed, Nov 14

rjmccall added a comment to D54539: [CodeGen] Replace '@' characters in block descriptors' symbol names with '\1' on ELF targets..

Oh, I see, sorry. Please rename these functions to make their purpose clear.

Wed, Nov 14, 4:36 PM · Restricted Project
rjmccall accepted D54166: [AST] Store the string data in StringLiteral in a trailing array of chars.

LGTM.

Wed, Nov 14, 4:34 PM · Restricted Project
rjmccall added a comment to D54166: [AST] Store the string data in StringLiteral in a trailing array of chars.

IIRC, abbreviations just silently don't take effect if the record doesn't conform; so things will appear to work, but the size on disk will be bigger.

I looked at where the abbreviations are defined, and it seems that the only abbreviations for
statements/expressions are for DeclRefExpr, IntegerLiteral, CharacterLiteral
and ImplicitCastExpr (grep for EmitAbbrev in Serialization/,
for some reasons they are emitted in WriteDeclAbbrev()...).

And indeed changing the serialization format of CharacterLiteral triggers various assertions
because of the abbreviation. Therefore unless I am missing something no other statement/expression
has currently an abbreviation. I suspect therefore that someone could go wild and cut the on-disk
size of the serialization format significantly here.

I looked at the size of the generated pch for all of Boost, and I am only seeing an increase of
about 8k, which is entirely attributable to the fact that I am adding one field to the serialization
format. I can rework it to remove this additional field if needed.

Wed, Nov 14, 4:33 PM · Restricted Project
rjmccall added inline comments to D54489: Implement -frecord-command-line (-frecord-gcc-switches).
Wed, Nov 14, 4:30 PM
rjmccall added a comment to D52440: Emit lifetime markers for temporary function parameter aggregates.

This is problematic because we're not necessarily in a scope that usefully limits the duration of cleanups — we don't push full-expression scopes when emitting an arbitrary statement. Probably we should, but we don't.

If you'd like to take a look at solving that problem first, that would be great.

Sure I'd like to take a look at this, but I'm not familiar with the code gen module. Can you point me to where I should start looking to understand and figure out where to add scopes?

Wed, Nov 14, 1:35 PM
rjmccall added a comment to D54539: [CodeGen] Replace '@' characters in block descriptors' symbol names with '\1' on ELF targets..

This should not be changing the actual @-encoding value. If we want to use the @-encoding in the symbol name, we can change it when naming the symbol, provided that \1 is not legal in the @encoding.

Wed, Nov 14, 1:11 PM · Restricted Project
rjmccall added a comment to D54166: [AST] Store the string data in StringLiteral in a trailing array of chars.

IIRC, abbreviations just silently don't take effect if the record doesn't conform; so things will appear to work, but the size on disk will be bigger.

Wed, Nov 14, 12:38 PM · Restricted Project
rjmccall added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Wed, Nov 14, 12:25 PM · Restricted Project
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Wed, Nov 14, 12:12 PM
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Wed, Nov 14, 9:57 AM
rjmccall added a comment to D52674: [AST] Add Obj-C discriminator to MS ABI RTTI.

I'm not worried about the mangler being re-used for multiple declarations, I'm worried about a global flag changing how we mangle all components of a type when we only mean to change it at the top level.

Hmm, but don't we want it to affect all components? For example, consider something like:

@interface I
@end

template <class T> class C {};

void f();
void g() {
  try {
    f();
  } catch (C<I> *) {
  }
}

I would say that we want the RTTI for C<I> * to have the discriminator for I.

Wed, Nov 14, 12:47 AM
rjmccall added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Wed, Nov 14, 12:41 AM · Restricted Project

Tue, Nov 13

rjmccall added a comment to D52674: [AST] Add Obj-C discriminator to MS ABI RTTI.

I'm not worried about the mangler being re-used for multiple declarations, I'm worried about a global flag changing how we mangle all components of a type when we only mean to change it at the top level.

Tue, Nov 13, 4:59 PM
rjmccall added a comment to D54489: Implement -frecord-command-line (-frecord-gcc-switches).

I realize that you're probably striving for option compatibility with gcc, but continuing to name it -frecord-gcc-switches when it actually records Clang switches seems weird to me. It almost sounds like something that would dump gcc equivalents of all Clang options, or maybe let you know which Clang options you've used that match gcc options. Either way, by the name -- if you aren't familiar with the gcc option -- it doesn't read like it records Clang options.

Would it be that bad to name it -frecord-clang-switches? Or just -frecord-switches?

I agree, and this was my original plan, but then I noticed that Clang already implements -grecord-gcc-switches and so I decided to mirror the naming for the -f variant as well.

If anything I think dropping the -gcc- altogether would make the most sense. I don't understand why GCC includes it in the first place.

Tue, Nov 13, 4:40 PM
rjmccall accepted D54055: CGDecl::emitStoresForConstant fix synthesized constant's name.

Seems reasonable to me.

Tue, Nov 13, 4:05 PM
rjmccall added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Tue, Nov 13, 3:41 PM · Restricted Project
rjmccall added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Tue, Nov 13, 11:34 AM · Restricted Project
rjmccall added inline comments to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".
Tue, Nov 13, 10:51 AM · Restricted Project
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Tue, Nov 13, 9:58 AM
rjmccall added a comment to D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)".

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my head. I seem to recall some discussion about trivial_abi not implying/being strong enough for all the cases that trivial_relocatable sounds like it covers? Do you happen to remember the distinction there (the summary in this patch ("the warranting attribute [[trivially_relocatable]], which is similar in spirit to [[trivial_abi]], in that it lets the programmer communicate back to the compiler that a certain user-defined type should be assumed to have this property even though it would not naturally have the property all else being equal.") doesn't sound like what I remember - but my memory is hazy)?

Tue, Nov 13, 9:27 AM · Restricted Project

Mon, Nov 12

rjmccall added a comment to D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators.

Seems reasonable to me.

Mon, Nov 12, 3:03 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Mon, Nov 12, 11:00 AM

Fri, Nov 9

rjmccall added a comment to D52674: [AST] Add Obj-C discriminator to MS ABI RTTI.

@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.

As I see it, there are a few ways to proceed here:

  • The approach in D53546 (cleaned up a bit). I think it's pretty ugly, but you may have suggestions on how to do it better.
  • In the mangleType overload for ObjCObjectPointerType, skipping the generic mangleType dispatch and going directly to either the ObjCObjectType or ObjCInterfaceType overloads. That avoids the ugliness in the generic mangleType, but it also requires us to duplicate some logic from it in the ObjCObjectPointerType overload, which doesn't seem great either.
  • Maintaining ForRTTI state in the mangler instead of threading a parameter through (I'm generally not a fan of state like that, but it might be cleaner than the threading in this case?)
  • Just sticking with what I'm doing in this patch.

    What do you think?

Sorry for the delay. I think duplicating the dispatch logic for ObjCObjectPointerType is probably the best approach; the pointee type will always an ObjCObjectType of some sort, and there are only two kinds of those.

Sorry, I'm still not sure how this will work.

Duplicating the dispatch logic for ObjCObjectPointerType ends up looking like P8114, which is fine. However, when we're actually mangling RTTI or RTTI names, we're still going through the main mangleType(QualType, SourceRange, QualifierMangleMode) overload, which still requires us to thread ForRTTI through that function, which still requires us to duplicate the switch in that function (to handle the ForRTTI case, since the other switch is generated via TypeNodes.def and not easily modifiable), which is the main ugliness in my opinion. Do you also want me to add special dispatching to mangleCXXRTTI and mangleCXXRTTIName to just call the ObjCObjectPointerType function directly when they're dealing with that type? That's certainly doable, but at that point just keeping some state around in the demangler starts to feel cleaner, at least to me.

Fri, Nov 9, 9:51 AM
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Fri, Nov 9, 9:42 AM

Thu, Nov 8

rjmccall accepted D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type.

LGTM.

Thu, Nov 8, 7:21 PM

Wed, Nov 7

rjmccall added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to support linking object files. Maybe I misunderstand the distinction between linkage and visibility, but it seems reasonable that a user would want to have e.g. a non-kernel function participate in static linking, but not be preemptible in the final shared object. The intention with this patch is to allow this with something like -fvisibility hidden without disrupting kernel symbols, which must appear in the dynsym for the reasons mentioned earlier in the thread.

Wed, Nov 7, 10:45 PM

Tue, Nov 6

rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

Not out of line with other features that significantly break with what's expressible. But the easy alternative to storing the intermediate "type" in the AST is to just provide a global function that can compute it on demand.

Yeah, it might just be simpler to go the route of not storing the computation semantic in the AST, at least for now. That's fairly similar to the solution in the patch, so I feel a bit silly for just going around in a circle.

To make that work I think the patch needs some changes, though. There should be a function in FixedPointSemantics to find the common full-precision semantic between two semantics, and the getFixedPointSemantics in ASTContext should be amended to take integer types (or another method should be provided for this, but that one already exists). I think that the FixedPointConversions method should also be embedded into the rest of UsualArithmeticConversions as there shouldn't be any need to have it separate. You still want to do the rvalue conversion and other promotions, and the rules for fixed-point still fit into the arithmetic conversions, even in the spec.

EmitFixedPointConversion should be changed to take FixedPointSemantics rather than QualType. This has a couple of downsides:

  • It can no longer handle floating point conversions. They would have to be handled in EmitScalarConversion.
  • Conversion from integer is fine, but conversion to integer cannot be generalized away with the fixed-point semantics as they are currently, as that kind of conversion must round towards zero. This requires a rounding step for negative numbers before downscaling, which the current code does not do. Is there a better way of generalizing this?

FixedPointSemantics is free to do whatever is convenient for the representation; if it helps to make it able to explicitly represent an integral type — and then just assert that it's never in that form when it's used in certain places, I think that works. Although you might consider making a FixedPointOrIntegralSemantics class and then making FixedPointSemantics a subclass which adds nothing to the representation but semantically asserts that it's representing a fixed-point type.

It might just be simpler and a bit more general to add a DownscalingRoundsTowardZero field to FixedPointSemantics, which specifies that the source value should be rounded toward zero before downscaling. Then conversion routines could handle the integer case explicitly. I'm not sure if the field would be useful for anything else, though; it has a pretty specific meaning.

I think it's a bit odd to have FixedPointSemantics as a specialization of something else, since technically integers are a specialization of fixed-point values and not the other way around.

Tue, Nov 6, 10:09 AM · Restricted Project

Mon, Nov 5

rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

Not out of line with other features that significantly break with what's expressible. But the easy alternative to storing the intermediate "type" in the AST is to just provide a global function that can compute it on demand.

Yeah, it might just be simpler to go the route of not storing the computation semantic in the AST, at least for now. That's fairly similar to the solution in the patch, so I feel a bit silly for just going around in a circle.

To make that work I think the patch needs some changes, though. There should be a function in FixedPointSemantics to find the common full-precision semantic between two semantics, and the getFixedPointSemantics in ASTContext should be amended to take integer types (or another method should be provided for this, but that one already exists). I think that the FixedPointConversions method should also be embedded into the rest of UsualArithmeticConversions as there shouldn't be any need to have it separate. You still want to do the rvalue conversion and other promotions, and the rules for fixed-point still fit into the arithmetic conversions, even in the spec.

EmitFixedPointConversion should be changed to take FixedPointSemantics rather than QualType. This has a couple of downsides:

  • It can no longer handle floating point conversions. They would have to be handled in EmitScalarConversion.
  • Conversion from integer is fine, but conversion to integer cannot be generalized away with the fixed-point semantics as they are currently, as that kind of conversion must round towards zero. This requires a rounding step for negative numbers before downscaling, which the current code does not do. Is there a better way of generalizing this?
Mon, Nov 5, 10:28 PM · Restricted Project
rjmccall added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

Okay, that's interesting. And that dynamic linking step includes fairly unrestricted linking of OpenCL code to other OpenCL code, rather than just e.g. loading a single block of OpenCL code that exports a small, fixed interface? If so, then I accept that you need symbol visibility.

Mon, Nov 5, 8:43 PM
rjmccall added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

But do you want to support *dynamically* linking object files? Because that's what visibility is about.

Mon, Nov 5, 5:28 PM
rjmccall added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I agree with Richard that I'm not sure what the point of supporting frontend visibility settings in OpenCL is. If you want the "everything is internal to the image" optimization, presumably you can just infer visibility on everything in a pass over the IR.

Eventually we want to support linking object files, and setting the visibility in the IR won't be aware of any user specified visibility

Mon, Nov 5, 5:18 PM
rjmccall added a comment to D53153: [OpenCL] Mark kernel functions with default visibility.

I agree with Richard that I'm not sure what the point of supporting frontend visibility settings in OpenCL is. If you want the "everything is internal to the image" optimization, presumably you can just infer visibility on everything in a pass over the IR.

Mon, Nov 5, 4:40 PM
rjmccall accepted D53780: Fix bitcast to address space cast for coerced load/stores.

Thanks! LGTM.

Mon, Nov 5, 3:56 PM

Sun, Nov 4

rjmccall accepted D54048: [AST] Get aliased type info from an aliased TemplateSpecialization..

Great!

Sun, Nov 4, 11:02 PM

Sat, Nov 3

rjmccall added a comment to D54048: [AST] Get aliased type info from an aliased TemplateSpecialization..

That looks a lot better, thanks. Did you check whether any other tests needed adjustment? That's not uncommon from this kind of desugaring change.

Sat, Nov 3, 6:08 PM
rjmccall added inline comments to D53780: Fix bitcast to address space cast for coerced load/stores.
Sat, Nov 3, 12:00 PM
rjmccall added a comment to D54055: CGDecl::emitStoresForConstant fix synthesized constant's name.

Pfft, if I'm going to be held responsible for my own work, I'm in a lot of trouble. :)

Sat, Nov 3, 11:59 AM
rjmccall added a comment to D54048: [AST] Get aliased type info from an aliased TemplateSpecialization..

Changing how TemplateSpecializationType is declared in TypeNodes.def has way more implications than you're giving it credit for.

Sat, Nov 3, 11:52 AM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

I'm just concerned about adding a great deal of complexity to mainline Clang in order to support a language that might still be in major flux and which I feel is likely to be forced to re-embrace qualifiers in the language model. Maybe this work should happen in a branch for awhile, at least for superficial things — patches to e.g. generalize things at the IRGen level to handle address spaces in various C++ language features would still be welcome, since those are likely to be useful across language modes.

Sat, Nov 3, 11:41 AM
rjmccall added a comment to D54055: CGDecl::emitStoresForConstant fix synthesized constant's name.
In D54055#1286397, @jfb wrote:

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

It was done in CodeGenFunction::EmitAutoVarInit a while ago. I moved it since then, but it's the same thing. I'm happy to mangle it any other way. At the end of the day we just need some name for an (unnamed address) global which is synthesized from a function-local initialization. We could just take the mangled function name and append something to it.

Sat, Nov 3, 11:33 AM

Fri, Nov 2

rjmccall added a comment to D54055: CGDecl::emitStoresForConstant fix synthesized constant's name.

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

Fri, Nov 2, 10:27 PM
rjmccall accepted D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

Thanks, LGTM.

Fri, Nov 2, 10:18 AM
rjmccall accepted D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer.

LGTM.

Fri, Nov 2, 8:39 AM

Thu, Nov 1

rjmccall added inline comments to D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer.
Thu, Nov 1, 9:36 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

Okay. So the purpose of OpenCL C++ is to provide a non-unified model that allows you to easily run C++ code on the CPU.

It is just the second-order purpose.
A C++-based kernel language to program accelerators without C++ language extension, so that the code could also mostly run on a CPU without a specific compiler.

Thu, Nov 1, 9:35 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

I was really meaning "run on the CPU" and not "run on the GPU", it was not a typo from me. :-)
If there are no visible language extensions besides C++ classes, it is easier to run the kernel code on a CPU without any specific compiler support, with just a plain C++ compiler and just by changing the runtime to launch the kernel.
It was part of the design motivation to remove the alien keywords.
This is even more true with SYCL.

Thu, Nov 1, 11:24 AM
rjmccall added inline comments to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.
Thu, Nov 1, 11:18 AM
rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.
  1. You would have an inconsistency in either case, since e.g. numeric + otherwise always returns the same type as its operands, but this would not.

True, but the CompoundAssignOperator already has that inconsistency with ComputationResultType.

Thu, Nov 1, 11:03 AM · Restricted Project
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Thu, Nov 1, 9:34 AM
rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

Well, it could be passed around through most code as some sort of abstractly-represented intermediate "type" which could be either a QualType or a fixed-point semantics.

Sounds to me like you're describing a new Type that can contain an arbitrary fixed-point semantic :)

Thu, Nov 1, 9:26 AM · Restricted Project

Wed, Oct 31

rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

Well, maybe the cleanest solution would be to actually fold CompoundAssignOperator back into BinaryOperator and just allow BinaryOperator to optionally store information about the intermediate type of the computation and how the operands need to be promoted. That information could be elided in the common cases where it's trivial, of course.

That sounds like a fairly hefty refactor. Also, doing the fold would put the extra QualType info from CAO into BO, sure, but this cannot work for the full-precision case since we can't represent those with QualTypes. The information for the full precision 'type' would have to be stored separately anyway.

Wed, Oct 31, 9:45 PM · Restricted Project
rjmccall accepted D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC..

LGTM.

Wed, Oct 31, 9:24 PM
rjmccall accepted D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC..

Thanks. Still LGTM. :)

Wed, Oct 31, 4:31 PM
rjmccall added a comment to D53925: [modules] Defer emission of inline key functions..

Seems reasonable to me.

Wed, Oct 31, 1:13 PM

Tue, Oct 30

rjmccall added a comment to D52674: [AST] Add Obj-C discriminator to MS ABI RTTI.

@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.

As I see it, there are a few ways to proceed here:

  • The approach in D53546 (cleaned up a bit). I think it's pretty ugly, but you may have suggestions on how to do it better.
  • In the mangleType overload for ObjCObjectPointerType, skipping the generic mangleType dispatch and going directly to either the ObjCObjectType or ObjCInterfaceType overloads. That avoids the ugliness in the generic mangleType, but it also requires us to duplicate some logic from it in the ObjCObjectPointerType overload, which doesn't seem great either.
  • Maintaining ForRTTI state in the mangler instead of threading a parameter through (I'm generally not a fan of state like that, but it might be cleaner than the threading in this case?)
  • Just sticking with what I'm doing in this patch.

    What do you think?
Tue, Oct 30, 9:16 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is.

You are actually telling this to Khronos since a lot of Khronos members are on the LLVM mailing lists and are reading this right now... :-)

Tue, Oct 30, 8:40 PM
rjmccall accepted D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC..

Well, that's the old style, but we've been slowly moving to the camelCase style instead. Very, very slowly. I won't hold up your patch over it.

Tue, Oct 30, 6:10 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

So OpenCL C code has to be completely rewritten if it needs to be used as part of an OpenCL C++ program? And it doesn't really compose like a type if it's supposed to change how a variable is stored. What a terrible little mess they've made for themselves.

Fair point. I would like to allow regular OpenCL address space qualifiers as a Clang feature at least, in case we won't be able to alter the spec. But one problem is that the private address space qualifier keyword conflicts with the private class access modifier. I guess we can only allow the address space qualifiers with the __ prefix. So some existing code would have to be changed when ported to OpenCL C++, but it should be easier than rewriting using classes.

Tue, Oct 30, 3:00 PM
rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

The important difference would be that we separate the semantics of performing the conversion and the operation. I understand that adding a new type could be a bit more involved than baking the conversion into the operator, but I really do not enjoy seeing so much implicit, implementation-specific logic encapsulated in the same AST element. Anyone who wants to look at BinaryOperators with fixed-point types will need to know all of the details of how the finding the common type and conversion is done, rather than simply "it does an addition".

It's not just that adding a new type is "involved", it's that it's a bad solution. Those types can't actually be expressed in the language, and changing the type system to be able to express them is going to lead to a lot of pain elsewhere.

I did figure that it might be a bit of work to adapt other parts of the code to handle the new type, but I just prefer separating the concerns and being explicit about what work is performed. To me, the clearest way of doing that is by handling the conversions explicitly in the AST, and separately from the operators themselves. Also, not being able to deal in QualTypes for the common full precision type handling means that reusing the operator handling code in Sema won't be easy, or even possible. It would have to be computed in CreateBuiltinBinOp, since it's not possible to forward anything but QualType from the CheckXXXOperands functions.

If you don't think it's a good idea I'll concede, but then there's the question of how to get the full precision semantics into the operator (if we store it there). I guess the most straightforward path is something like:

  • In CreateBuiltinBinOp, do the normal Check function to get the result type
  • If the result is a fixed-point type, go into a separate code path
  • Ask a method for the common FixedPointSemantics of the given LHS and RHS
  • Build the correct BinaryOperator subclass.

    I need to think about this to see if our downstream implementation can be adapted to work with this design.

    Compound assignments are already their own subclass, though. Unless the full precision semantic information is simply baked into the regular BinaryOperator, it would require two new subclasses: one for normal full precision operators and one for compound ones? Wouldn't adding this require new hooks and code paths in visitors, even though there's really nothing different about the operator?

    The type information that CompoundAssignOperator stores today is rather similar to what a full precision operator would need, though it would need to store a FixedPointSemantics instead.
Tue, Oct 30, 2:57 PM · Restricted Project
rjmccall added a comment to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

Thanks, and I agree with renaming the existing option.

Tue, Oct 30, 2:48 PM
rjmccall added a comment to D53860: [SemaCXX] Don't check base's dtor is accessible.

That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make their destructors protected in order to prevent types from being constructed except as base sub-objects, which is still valid here. But at the same time, at a low level, we are directly accessing a protected destructor from a context that is not code actually defined in a subclass.

Tue, Oct 30, 2:21 AM

Mon, Oct 29

rjmccall committed rC345536: In swiftcall, don't merge FP/vector types within a chunk..
In swiftcall, don't merge FP/vector types within a chunk.
Mon, Oct 29, 1:36 PM
rjmccall committed rL345536: In swiftcall, don't merge FP/vector types within a chunk..
In swiftcall, don't merge FP/vector types within a chunk.
Mon, Oct 29, 1:36 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

Unlikely, since address spaces are provided in a different way in OpenCL C++ vs OpenCL C.

OpenCL C provides qualifiers such as global as part of the language. OpenCL C++ provides template classes such as cl::global<T> through a header file.

Mon, Oct 29, 10:53 AM
rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

I don't think we should add *types* just for this, but if you need to make a different kind of BinaryOperator that represents that the semantics aren't quite the same (the same way that the compound assignments have their own subclass), that seems natural to me.

I don't know if adding a new operator kind was what I had in mind. The operator hasn't changed, it's still a normal binary operator. Compound assignments are their own operator with its own syntax; it doesn't really strike me as the same thing.

Mon, Oct 29, 10:43 AM · Restricted Project

Sun, Oct 28

rjmccall added inline comments to D53780: Fix bitcast to address space cast for coerced load/stores.
Sun, Oct 28, 10:56 PM

Fri, Oct 26

rjmccall accepted D53714: [AST] Only store the needed data in SwitchStmt..

LGTM.

Fri, Oct 26, 9:31 PM · Restricted Project
rjmccall added a comment to D53738: [Fixed Point Arithmetic] Fixed Point Addition.

I want to float the idea again of adding an AST type to encapsulate an arbitrary fixed-point semantic and using that as the 'common type' for binary operations involving fixed-point types. This would enable UsualArithmeticConversions to handle fixed-point types similarly to how it does today (find the 'common' full precision type, implicitly cast the LHS and RHS if necessary). There is one new thing that it would have to do; the result after the operation should not be the full precision type, but rather be casted to the operand type of highest rank. I don't think the existing code in SemaExpr can handle the case of adding an implicit cast after the operation. I don't think it should be hard to add, though.

Fri, Oct 26, 9:30 PM · Restricted Project
rjmccall accepted D53605: [AST] Refactor PredefinedExpr.

LGTM.

Fri, Oct 26, 9:21 PM · Restricted Project
rjmccall added inline comments to D53764: [OpenCL] Enable address spaces for references in C++ .
Fri, Oct 26, 9:21 PM
rjmccall added inline comments to D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC..
Fri, Oct 26, 9:06 PM
rjmccall added a comment to D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC..

This should at least be named emitScalarConstant.

Fri, Oct 26, 9:06 PM
rjmccall added a comment to D53705: [OpenCL] Postpone PSV address space diagnostic.

I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is.

Fri, Oct 26, 9:00 PM
rjmccall added a comment to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

So, three points:

Fri, Oct 26, 5:43 PM
rjmccall accepted D53715: [AST] Only store the needed data in WhileStmt..

LGTM.

Fri, Oct 26, 4:22 PM · Restricted Project

Thu, Oct 25

rjmccall accepted D53607: [AST] Only store the needed data in IfStmt..

LGTM.

Thu, Oct 25, 10:31 PM · Restricted Project
rjmccall added a comment to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

Oops, sorry for the unedited comment; it's fixed on Phab.

Thu, Oct 25, 10:29 PM
rjmccall added a comment to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

This seems like an unreasonably long flag name. Can you try to find a shorter name for it?

-fsanitize-poison-extra-operator-new?
Not as explicit, but maybe ok if documented?

-fsanitize-address-poison-array-cookie?

I strongly dislike this one because "poison array cookie", in general, is always enabled (it's actually triggered by a runtime flag). This flag is about poisoning it in more cases (cases where the standard doesn't completely disallow accesses to the cookie, so we have to have a flag and can't enable it all the time).

Thu, Oct 25, 10:24 PM
rjmccall accepted D53609: [AST] Don't store data for GNU range case statement if not needed..

LGTM.

Thu, Oct 25, 9:46 PM · Restricted Project

Wed, Oct 24

rjmccall added inline comments to D53609: [AST] Don't store data for GNU range case statement if not needed..
Wed, Oct 24, 6:15 PM · Restricted Project
rjmccall added inline comments to D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC..
Wed, Oct 24, 5:41 PM
rjmccall added a comment to D53607: [AST] Only store the needed data in IfStmt..

Yeah, I agree that changing child order is problematic.

Wed, Oct 24, 5:25 PM · Restricted Project
rjmccall accepted D53610: [AST] Check that GNU range case statements are correctly imported..

LGTM.

Wed, Oct 24, 5:23 PM · Restricted Project
rjmccall added inline comments to D53605: [AST] Refactor PredefinedExpr.
Wed, Oct 24, 5:22 PM · Restricted Project
rjmccall accepted D53604: [AST] Widen the bit-fields of Stmt to 8 bytes.

LGTM.

Wed, Oct 24, 5:17 PM · Restricted Project

Tue, Oct 23

rjmccall added a comment to D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1.

This seems like an unreasonably long flag name. Can you try to find a shorter name for it?

-fsanitize-poison-extra-operator-new?
Not as explicit, but maybe ok if documented?

Tue, Oct 23, 9:56 AM
rjmccall added a reviewer for D53295: Mark store and load of block invoke function as invariant.group: Prazek.

Btw, blocks w/o captures are already optimized into regular calls?

Tue, Oct 23, 9:43 AM
rjmccall accepted D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast.

LGTM.

Tue, Oct 23, 9:24 AM · Restricted Project