LGTM

Yes, that's the right fix, although you might also consider adding a getObjectType() to CXXMemberCallExpr.

LGTM

LGTM with one minor request.

Thanks!

Thanks.

I'm fine with the idea of targets making unsupported CCs hard errors.

Minor comment then LGTM

Sounds good.

In D62584#1585091, @Anastasia wrote:

In D62584#1583340, @rjmccall wrote:

Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it.

Ok, regarding address space qualifiers in template instantiation - is it still ok that we ignore them here in this patch?

Thanks, LGTM!

Thanks!

LGTM

Thanks, just a few minor comment requests now.

Oh, I see you've already answered that. I think it's fine to just leave this testing debug output if generated optimized output doesn't affect it; the bulk of our regression testing is with assertions-enabled compilers anyway.

Seems reasonable to me.

Okay.

Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it.

In D64083#1583109, @rjmccall wrote:

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

A qualifier "outside" the block-pointer type is telling you what address space the object which holds the block pointer is in, which is a completely different thing. Instead of __global int (^block4)(void) = ^{ return 0; };, you need to write int (^__global block4)(void) = ^{ return 0; };.

If you're interested in working on this, great. I actually think there's zero reason to emit a non-null argument here on any target unless we're going to use the destructor as the function pointer — but we can do that on every Itanium target, so we should. So where we want to be is probably:

• use the complete-object destructor and the object pointer as its argument when destroying a (non-array) object of C++ class type
• otherwise use a custom function that materializes the object pointer on its own and uses a null pointer as the argument

Current patch LGTM

(Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.)

It's good to have a lambda test, but that one isn't actually testing the lambda path — the place the diagnostic will trigger is just the normal function-prototype path, just originally within a lambda. You can do something like this:

There are some code paths that I think are common between the parser and template instantiation, like BuildPointerType and BuildReferenceType, but if you want to do context-sensitive inference that might not be good enough.

I agree that we really shouldn't use most of those — we shouldn't have null types or null child expressions anywhere in the AST. (Accessors might return null for *optional* children, of course, but that's different.) And yeah, a big part of that would be having an error type that could be used in various places (instead of e.g. defaulting to int).

I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid. For case, which has external requirements (e.g. not being a duplicate value) not entirely dissimilar to a declaration, I think that means having an "invalid" flag; arbitrary tools already can't rely on the expression being constant-evaluable because of templates, although granted many tools might never look at template patterns. For other things (e.g. a binary operator) maybe that means using different classes (e.g. InvalidBinaryOperator) so that tools looking at an apparently well-typed expression don't need to consider totally invalid possibilities.

Thanks, that looks great. A few more requests and then this will be ready to go, I think.

I wouldn't favor adding something really obscure that was only useful for clang, but I think builtins to set and clear mask bits while promising to preserve object-reference identity would be more generally useful for libraries. For example, there might be somewhere in libc++ that could take advantage of this.

Thanks!

In D63846#1574311, @vzakhari wrote:

In D63846#1574302, @rjmccall wrote:

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Sorry, I have one particular question about clang/test/CodeGenOpenCLCXX/addrspace-of-this.cl:
Test the address space of 'this' when invoking copy-constructor.
COMMON: [[C1GEN:%c1.ascast[0-9]*]] = addrspacecast %class.C* %c1 to %class.C addrspace(4)*

This check seems to rely on %c1 name already. I guess the matching may go off, if we do not use actual names on the right hand side of the assignment. Should I do anything about the right hand side, or just use a generic wildcard on the left hand side?

Okay.

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Yes, I think you can commit.

Please don't check IR names in test output. That actually includes anonymous names like %2; these should always be tested with FileCheck variables. I suggest using %.* as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems with accidentally matching too much.

I'm fine with proceeding with your best guess about what the ABI should be.

I would be opposed to any addition of a -f flag for this absent any evidence that it's valuable for some actual C code; this patch appears to be purely speculative. I certainly don't think we should be adding it default-on. Your argument about alignment is what I was referring to when I mentioned that this is enforcing alignment restrictions in places we traditionally and intentionally haven't.

Okay. Akira, do you have any interest in looking into this as a general block optimization?

In D64128#1569836, @hfinkel wrote:

In D64128#1569817, @rjmccall wrote:

The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.", and we do have to honor that. The standard allows that "the result ... might not point to an entity of the referenced type", but when in fact it's guaranteed to do so (i.e. it's not just a coincidental result of an implementation decision like the exact address of a global variable — no "guessing"), I do think we have an obligation to make it work. And on a practical level, there has to be *some* way of playing clever address tricks in the language in order to implement things like allocators and so forth. So this makes me very antsy.

I don't disagree. But I believe the question is if we have:

int *x = malloc(4);
int *y = malloc(4);
if (x & ~15 == y) {
  *(x & ~15) = 5; // Is this allowed, and if so, must the compiler assume that it might set the value of *y?
}

I certainly agree that we must allow the implementation of allocators, etc. But allocators, I think, have the opposite problem. They actually have some large underlying objects (from mmap or whatever), and we want the rest of the system to treat some subobjects of these larger objects as though they were independent objects of some given types. From the point of view of the allocator, we have x, and we have void *memory_pool, and we need to allow x & N to point into memory_pool, but because, from the allocator's perspective, we never knew that x didn't point into memory_pool (as, in fact, it likely does), that should be fine (*).

There might be more of an issue, for example, if for a given object, I happen to know that there's some interesting structure at the beginning of its page (or some other boundary).

Thanks, LGTM.

The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.", and we do have to honor that. The standard allows that "the result ... might not point to an entity of the referenced type", but when in fact it's guaranteed to do so (i.e. it's not just a coincidental result of an implementation decision like the exact address of a global variable — no "guessing"), I do think we have an obligation to make it work. And on a practical level, there has to be *some* way of playing clever address tricks in the language in order to implement things like allocators and so forth. So this makes me very antsy.

Thanks, this looks good to me.

I agree with Eli that this isn't obviously a legal transformation. llvm.ptrmask appears to make semantic guarantees about e.g. the pointer after the mask referring to the same underlying object, which means we can only safely emit it when something about the source program makes that guarantee. It's not at all clear that C does so for an expression like (T*) ((intptr_t) x & N).

Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here.

Thanks, looks much better.

Oh, sorry, I had completely forgotten about that.

Are the CaseStmts being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid.

Great, thank you. Yaxun, are you planning to pick this back up? I know it's been a long time.

I'm not sure we've ever written down what the semantics of the block capture are actually supposed to be in this situation. I guess capturing a reference is the right thing to do? Is that what we generally do if this is a POD type?

In D63856#1561132, @erik.pilkington wrote:

In D63856#1561112, @rjmccall wrote:

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

https://reviews.llvm.org/D27607

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

My understanding is that Tim Northover sometimes reviews patches to compiler-rt, even if he's not an official owner; CC'ing him.

Thanks! Still don't feel qualified to actually approve compiler-rt changes, though.

In D63451#1552609, @rsmith wrote:

In D63451#1549563, @rjmccall wrote:

Can this attribute not be applied to a base class, or to a type?

The standard attribute forbids that right now. We could add a custom attribute that permits it, but we're required to diagnose application of the standard attribute to a type -- though a warning would suffice to satisfy that requirement. (Because this gives "same as a base class" layout, adding it to a base class wouldn't have any effect right now, but we could certainly say that the attribute on a class type also implies the class is not POD for the purpose of layout, to permit tail padding reuse.)

Alright, thanks. I agree that per the documentation this should be sufficient.

This optimization is disabled for functions not in COMDAT sections? Is that documented somewhere?

Can this attribute not be applied to a base class, or to a type? I think every time I've seen someone get bitten by the unique-address rule, it was because they had a base class that deleted some constructors (or something like that) and which was a base of a million different types; I'm sure the language model they'd prefer would be to just add an attribute to that one type instead of chasing down every place where they declared a field of the type.

In D62088#1549139, @leonardchan wrote:

In D62088#1537470, @bjope wrote:

Please add test cases for scale=0 and scale=width as I assume those need special handling (UB right now?).
And if scale=0 and scale=width needs special handling, then I guess scale=1 and scale=width-1 are new boundary values so I maybe it would be nice to have tests for those scales as well.

Added. For a scale of 0 we only return the integer result. For widths > 60 or 28, we return INT_MAX to represent an error since we need to represent 10 << scale in 64/32 bits for this to work. So the scale boundaries are [0,60] for the 64 bit version and [0,28] for the 32 bit version. We could technically increase this to go up to 63/31, although to get the precise result would require a bigger buffer (using more 128 bit ints) which would complicate things further.

For the 64 bit function, I realized that in order to get the precise result without a 128 bit int would require implementing division by 10 in 2 64 bit ints. I eventually found a way to do this and get the precise result, but am not sure if it should be included in this patch since it would be adding another large function. Perhaps it could be readded in a followup? For now, I removed the functions we fallback to if 128 bit ints aren't supported and only define the function if 128 bits are enabled.

In D59744#1549229, @efriedma wrote:

If we're going to insert emms instructions automatically, it doesn't really make sense to do it in the frontend; the backend could figure out the most efficient placement itself. (See lib/Target/X86/X86VZeroUpper.cpp, which implements similar logic for AVX.) The part I'd be worried about is the potential performance hit from calling emms in places where other compilers wouldn't, for code using MMX intrinsics.