rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (266 w, 12 h)

Recent Activity

Yesterday

rjmccall added a comment to D42728: Add more warnings for implict conversions (e.g. double truncation to float)..

The diagnostic text for this warning is misleading at best when the conversion is actually a truncation of the result of a compound assignment.

OK, at the moment we have:

*b += a; // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}

But you'd like to see something like:

*b += a; // expected-warning {{truncation of the result of a compound assignment loses floating-point precision: 'double' to 'float'}}

Right?

Fri, Feb 23, 9:33 AM

Thu, Feb 22

rjmccall added a comment to D42728: Add more warnings for implict conversions (e.g. double truncation to float)..

I'm not sure I understand you. There is a special branch in CheckImplicitConversion related to FloatingPoint:

if (SourceBT && SourceBT->isFloatingPoint()) {
  // ...and the target is floating point...
  if (TargetBT && TargetBT->isFloatingPoint()) {
    // ...then warn if we're dropping FP rank.

And that's exactly our case. How can knowledge about "part of a compound assignment" improve the diagnostic here? Please, clarify.

Thu, Feb 22, 12:15 PM
rjmccall added inline comments to D34367: CodeGen: Fix address space of indirect function argument.
Thu, Feb 22, 12:12 PM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Thu, Feb 22, 11:55 AM

Wed, Feb 21

rjmccall accepted D43586: CodeGen: handle blocks correctly when inalloca'ed.

I hate inalloca so much. Okay.

Wed, Feb 21, 1:32 PM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Wed, Feb 21, 1:30 PM
rjmccall added inline comments to D34367: CodeGen: Fix address space of indirect function argument.
Wed, Feb 21, 12:14 PM
rjmccall added inline comments to D34367: CodeGen: Fix address space of indirect function argument.
Wed, Feb 21, 8:03 AM
rjmccall added a comment to D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses.

I think zero would serve better as the unknown-size value. People who are not aware of TBAA internals would guess that since zero-sized accesses make no sense, they are likely to have some special meaning. Similarly, for code that is supposed to process the size fields of access descriptors zero would be an obvious "illegal size value". In contrast, UINT64_MAX is just a very large number that doesn't hint anything on its special purpose.

Wed, Feb 21, 7:36 AM · Restricted Project

Mon, Feb 19

rjmccall added a comment to D43430: Omit nullptr check for sufficiently simple delete-expressions.

Discourse nitpick: I would encourage you to just use the ordinary phrase "null pointer", or just "null", when referring to a pointer value that happens to be null and to reserve "nullptr" for *statically* null pointers, especially the nullptr expression.

Mon, Feb 19, 8:47 AM
rjmccall added a comment to D43467: Allowing more [[]] attributes in C.

LGTM.

Mon, Feb 19, 8:25 AM
rjmccall accepted D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses.

I'm fine with that plan. LGTM.

Mon, Feb 19, 8:25 AM · Restricted Project

Fri, Feb 16

rjmccall accepted D43181: [CodeGen] Initialize large arrays by copying from a global.

LGTM, thanks!

Fri, Feb 16, 10:00 AM · Restricted Project

Thu, Feb 15

rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Thu, Feb 15, 12:09 PM
rjmccall added a comment to D34367: CodeGen: Fix address space of indirect function argument.

That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation requirement.

Thu, Feb 15, 10:29 AM

Wed, Feb 14

rjmccall added inline comments to D43089: clang: Add ARCTargetInfo.
Wed, Feb 14, 10:25 AM

Tue, Feb 13

rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Tue, Feb 13, 2:54 PM
rjmccall added a comment to D42728: Add more warnings for implict conversions (e.g. double truncation to float)..

I removed the ambiguity with changes in CheckImplicitConversion (required by rksimon): now it's absolutely clear what was changed.
And I minimized changes in diagnostics(required by rjmccall): the only float-double truncation was changed. As result only two tests were changed. Later we could easily extend the diagnostic updates if it will be necessary (but inside another patch).

Tue, Feb 13, 12:24 PM
rjmccall accepted D43187: [AST] Refine the condition for element-dependent array fillers.

LGTM.

Tue, Feb 13, 10:54 AM · Restricted Project
rjmccall added inline comments to D43181: [CodeGen] Initialize large arrays by copying from a global.
Tue, Feb 13, 10:48 AM · Restricted Project

Mon, Feb 12

rjmccall added inline comments to D43181: [CodeGen] Initialize large arrays by copying from a global.
Mon, Feb 12, 9:20 AM · Restricted Project

Sun, Feb 11

rjmccall accepted D41553: Support parsing double square-bracket attributes in ObjC.

No, this LGTM.

Sun, Feb 11, 11:08 PM

Fri, Feb 9

rjmccall added a comment to D42728: Add more warnings for implict conversions (e.g. double truncation to float)..

Hmm. I think if you're going to push for this, you need to put more time into making sure that the diagnostics are good, and most them seem pretty questionable.

Fri, Feb 9, 3:33 PM

Thu, Feb 8

rjmccall accepted D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation..

No worries. LGTM!

Thu, Feb 8, 6:32 PM
rjmccall added a comment to D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation..

Oh. It's not a good idea to try to match exact local IR names like this for two reasons:

  • First, many IR names are different in builds with and without assertions.
  • Second, it's pretty susceptible to innocuous changes in output.
Thu, Feb 8, 4:17 PM
rjmccall added a comment to D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation..

I'm somewhat surprised LLVM doesn't already canonicalize this, but ok.

Thu, Feb 8, 11:18 AM
rjmccall added a comment to D42530: Clang permits assignment to vector/extvector elements in a const method.

Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM.

Thu, Feb 8, 11:15 AM
rjmccall accepted D43013: ASan+operator new[]: Fix operator new[] cookie poisoning.

Okay, thanks. LGTM.

Thu, Feb 8, 11:03 AM

Wed, Feb 7

rjmccall accepted D42614: AST: support ObjC lifetime qualifiers in MS ABI.

@rjmccall, I've updated the approach and no longer abuse the existing decoration styles. This uses a custom namespace with artificial types to differentiate the types. I've also ensured that the parameter types do not encode the type information.

Wed, Feb 7, 10:43 PM
rjmccall added a comment to D43013: ASan+operator new[]: Fix operator new[] cookie poisoning.

I don't understand why your description of this patch mentions the void* placement new[] operator. There's no cookie to poison for that operator.

Wed, Feb 7, 12:13 PM

Tue, Feb 6

rjmccall committed rL324377: Pass around function pointers as CGCallees, not bare llvm::Value*s..
Pass around function pointers as CGCallees, not bare llvm::Value*s.
Tue, Feb 6, 10:55 AM
rjmccall committed rC324377: Pass around function pointers as CGCallees, not bare llvm::Value*s..
Pass around function pointers as CGCallees, not bare llvm::Value*s.
Tue, Feb 6, 10:55 AM

Fri, Feb 2

rjmccall added a comment to D42530: Clang permits assignment to vector/extvector elements in a const method.

No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former.

Fri, Feb 2, 12:05 PM
rjmccall added a comment to D42530: Clang permits assignment to vector/extvector elements in a const method.

If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in the code.

Fri, Feb 2, 8:44 AM

Thu, Feb 1

rjmccall accepted D42811: [CodeGen][va_args] Correct Vector Struct va-arg 'in_reg' code gen.

Okay. LGTM.

Thu, Feb 1, 11:13 PM
rjmccall added a comment to D42530: Clang permits assignment to vector/extvector elements in a const method.

That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value.

Thu, Feb 1, 11:17 AM

Wed, Jan 31

rjmccall added a comment to D42768: AST: support SwiftCC on MS ABI.

No, I mean things like void foo(__attribute__((swiftcall)) void (*fnptr)());.

Wed, Jan 31, 11:27 PM
rjmccall added a comment to D42768: AST: support SwiftCC on MS ABI.

Is demangling "correctly" really a more important goal here than not spuriously failing when presented with a swiftcall function type in a non-top-level position? I don't know that there was anything wrong with the previous patch's approach to this if we're just going to punt on handling it properly because MS happens to not have an official mangling for it.

Wed, Jan 31, 8:48 PM

Tue, Jan 30

rjmccall accepted D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack.

Oh, that makes much more sense, thanks.

Tue, Jan 30, 10:50 PM

Mon, Jan 29

rjmccall added inline comments to D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack.
Mon, Jan 29, 2:34 PM
rjmccall added inline comments to D41553: Support parsing double square-bracket attributes in ObjC.
Mon, Jan 29, 1:50 PM
rjmccall added a comment to D41543: [Transforms] Propagate new-format TBAA tags on simplification of memory-transfer intrinsics.

This seems reasonable, but I don't generally review LLVM patches.

Mon, Jan 29, 10:10 AM
rjmccall added a comment to D42614: AST: support ObjC lifetime qualifiers in MS ABI.

Hmm, the only thing that we could fake would be namespaces on the parameter types. Is that better? I'm not tied to re-using the existing mangling.

Mon, Jan 29, 10:10 AM

Sat, Jan 27

rjmccall added a comment to D42614: AST: support ObjC lifetime qualifiers in MS ABI.

It's not just that functions can't be overloaded on the parameter-variable type qualifier — it's not part of the function type at all, just like making a parameter 'const int' instead of 'int' is not part of the function type. I understand that MSVC mangles some things that really shouldn't be mangled, but I would greatly prefer if you could make an exception for this.

Sat, Jan 27, 9:30 PM
rjmccall accepted D41677: Change memcpy/memove/memset to have dest and source alignment attributes..

Still LGTM.

Sat, Jan 27, 9:24 PM

Fri, Jan 26

rjmccall added inline comments to D42530: Clang permits assignment to vector/extvector elements in a const method.
Fri, Jan 26, 11:14 AM

Thu, Jan 25

rjmccall added inline comments to D42530: Clang permits assignment to vector/extvector elements in a const method.
Thu, Jan 25, 11:19 AM
rjmccall accepted D42521: [CodeGen] Use the non-virtual alignment when emitting the base class subobject constructor.

LGTM.

Thu, Jan 25, 11:17 AM

Jan 24 2018

rjmccall added a comment to D42508: AST: support protocol conformances on id/class/interfaces in MS ABI.

The basic idea here seems fine to me; I'll leave David to review the details.

Jan 24 2018, 4:22 PM
rjmccall accepted D41539: [CodeGen] Decorate aggregate accesses with TBAA tags.

Okay, that works, thanks! LGTM.

Jan 24 2018, 3:47 PM · Restricted Project

Jan 22 2018

rjmccall accepted D42154: Don't generate inline atomics for i386/i486.

Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM.

Jan 22 2018, 5:29 PM
rjmccall added inline comments to D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses.
Jan 22 2018, 9:09 AM · Restricted Project
rjmccall added a comment to D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference.

This is definitely something that the personality function should handle. If we want to do heroic things in the absence of personality function support, we can, but the code should at least be written to be conditional on personality support.

Jan 22 2018, 8:49 AM
rjmccall added a comment to D41539: [CodeGen] Decorate aggregate accesses with TBAA tags.

Thank you. Maybe there should be an overload of EmitAggregateCopy that takes LValues? A lot of these cases start with an LValue on at least one side, and there are already some convenience functions to turn an Address into a naturally-typed LValue.

Jan 22 2018, 8:15 AM · Restricted Project
rjmccall added a comment to D42154: Don't generate inline atomics for i386/i486.
In D42154#977991, @wmi wrote:

The LLVM backend currently assumes every CPU is Pentium-compatible. If we're going to change that in clang, we should probably fix the backend as well.

With the patch, for i386/i486 targets, clang will generate more atomic libcalls than before, for which llvm backend will not do anything extra, so no fix is necessary in llvm backend for the patch to work.

Jan 22 2018, 8:03 AM
rjmccall accepted D41039: Add support for attribute "trivial_abi".

Looks great to me! Thanks for taking this on, it's a pretty major improvement for users.

Jan 22 2018, 7:51 AM
rjmccall accepted D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared..

LGTM.

Jan 22 2018, 7:49 AM
rjmccall added inline comments to D41553: Support parsing double square-bracket attributes in ObjC.
Jan 22 2018, 7:45 AM

Jan 12 2018

rjmccall closed D40569: Use default IR alignment for cleanup.dest.slot..
Jan 12 2018, 2:09 PM
rjmccall accepted D40569: Use default IR alignment for cleanup.dest.slot..

r322406

Jan 12 2018, 2:09 PM
rjmccall commandeered D40569: Use default IR alignment for cleanup.dest.slot..
Jan 12 2018, 2:08 PM
rjmccall committed rC322406: Allocate and access NormalCleanupDest with the natural alignment of i32..
Allocate and access NormalCleanupDest with the natural alignment of i32.
Jan 12 2018, 2:08 PM
rjmccall committed rL322406: Allocate and access NormalCleanupDest with the natural alignment of i32..
Allocate and access NormalCleanupDest with the natural alignment of i32.
Jan 12 2018, 2:08 PM
rjmccall accepted D40023: [RISCV] Implement ABI lowering.

Thanks. LGTM, but you should wait for Eli's sign-off, too.

Jan 12 2018, 1:02 PM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Jan 12 2018, 12:17 PM
rjmccall added inline comments to D40023: [RISCV] Implement ABI lowering.
Jan 12 2018, 10:25 AM
rjmccall accepted D41999: Refactor handling of signext/zeroext in ABIArgInfo.

Thanks, that looks great. I appreciate you doing this.

Jan 12 2018, 10:24 AM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Jan 12 2018, 9:41 AM
rjmccall added inline comments to D40023: [RISCV] Implement ABI lowering.
Jan 12 2018, 8:42 AM

Jan 9 2018

rjmccall added inline comments to D41553: Support parsing double square-bracket attributes in ObjC.
Jan 9 2018, 10:35 AM

Jan 8 2018

rjmccall accepted D41039: Add support for attribute "trivial_abi".

Thanks, looks good to me.

Jan 8 2018, 1:02 PM
rjmccall added a comment to D41539: [CodeGen] Decorate aggregate accesses with TBAA tags.

Yes, I think that's fine.

Jan 8 2018, 8:25 AM · Restricted Project
rjmccall added inline comments to D41039: Add support for attribute "trivial_abi".
Jan 8 2018, 8:20 AM

Jan 6 2018

rjmccall committed rC321957: Simplify the internal API for checking whether swiftcall passes a type….
Simplify the internal API for checking whether swiftcall passes a type…
Jan 6 2018, 10:30 PM
rjmccall committed rL321957: Simplify the internal API for checking whether swiftcall passes a type….
Simplify the internal API for checking whether swiftcall passes a type…
Jan 6 2018, 10:30 PM

Jan 5 2018

rjmccall added a comment to D41039: Add support for attribute "trivial_abi".

I'll trust Richard on the tricky Sema/AST bits. The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect. But we could clean that up later if it's significantly simpler to do it this way.

Jan 5 2018, 11:19 PM

Jan 4 2018

rjmccall added a comment to D41039: Add support for attribute "trivial_abi".

I had a discussion with Duncan today and he pointed out that perhaps we shouldn't allow users to annotate a struct with "trivial_abi" if one of its subobjects is non-trivial and is not annotated with "trivial_abi" since that gives users too much power.

Should we error out or drop "trivial_abi" from struct Outer when the following code is compiled?

struct Inner1 {
  ~Inner1(); // non-trivial
  int x;
};

struct __attribute__((trivial_abi)) Outer {
  ~Outer();
  Inner1 x;
};

The current patch doesn't error out or drop the attribute, but the patch would probably be much simpler if we didn't allow it.

I think it makes sense to emit an error if there is provably a non-trivial-ABI component. However, for class temploids I think that diagnostic should only fire on the definition, not on instantiations; for example:

template <class T> struct __attribute__((trivial_abi)) holder {
   T value;
   ~holder() {}
};
holder<std::string> hs; // this instantiation should be legal despite the fact that holder<std::string> cannot be trivial-ABI.

But we should still be able to emit the diagnostic in template definitions, e.g.:

template <class T> struct __attribute__((trivial_abi)) named_holder {
   std::string name; // there are no instantiations of this template that could ever be trivial-ABI
   T value;
   ~named_holder() {}
};

The wording should be something akin to the standard template rule that a template is ill-formed if it has no valid instantiations, no diagnostic required.

I would definitely like to open the conversation about the name of the attribute. I don't think we've used "abi" in an existing attribute name; usually it's more descriptive. And "trivial" is a weighty word in the standard. I'm not sure I have a great counter-proposal off the top of my head, though.

Jan 4 2018, 7:40 PM

Jan 3 2018

rjmccall added inline comments to D41553: Support parsing double square-bracket attributes in ObjC.
Jan 3 2018, 3:01 PM

Jan 2 2018

rjmccall added inline comments to D41553: Support parsing double square-bracket attributes in ObjC.
Jan 2 2018, 7:29 PM
rjmccall added a comment to D41562: [CodeGen] Generate TBAA info on passing arguments and returning values.

Aren't these always loads and stores from allocas? I would expect TBAA to be useless here because it's always strictly less informative than basic-AA, which knows that the access doesn't alias with anything.

Jan 2 2018, 4:29 PM · Restricted Project
rjmccall accepted D41547: [CodeGen] Fix TBAA info for accesses to members of base classes.

Ok.

Jan 2 2018, 4:26 PM · Restricted Project
rjmccall added a comment to D41539: [CodeGen] Decorate aggregate accesses with TBAA tags.

You're sure you just want a single TBAA tag on memcpy's? I would think that it might be useful to encode separate path information for the two operands.

Jan 2 2018, 4:26 PM · Restricted Project
rjmccall added a comment to D41633: [InstCombine] Remove unneeded VarArg casts..

I think at least one of the MIPS ABIs does the same thing with varargs as AArch64 on Darwin.

Jan 2 2018, 1:00 PM
rjmccall accepted D41677: Change memcpy/memove/memset to have dest and source alignment attributes..

I'm glad to hear that progress is finally happening on this.

Jan 2 2018, 12:47 PM

Dec 21 2017

rjmccall accepted D41433: Unit tests for TBAA metadata generation..

Interesting. Okay, LGTM.

Dec 21 2017, 11:23 PM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Dec 21 2017, 11:22 PM
rjmccall accepted D41394: [CodeGen] Support generation of TBAA info in the new format.

That's great, thanks. LGTM.

Dec 21 2017, 8:19 AM · Restricted Project

Dec 20 2017

rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Dec 20 2017, 9:25 PM
rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Dec 20 2017, 7:29 PM
rjmccall added a comment to D41452: [CodeGen] Fix access sizes in new-format TBAA tags.

LGTM.

Dec 20 2017, 7:19 PM · Restricted Project
rjmccall added a comment to D41394: [CodeGen] Support generation of TBAA info in the new format.

You can pass multiple -check-prefix arguments to FileCheck and it'll match all of them. You can use that to make your test change simpler: make the existing RUN check for both PATH and OLD-PATH and the new RUN check for both PATH and NEW-PATH, then change all the existing metadata matches to OLD-PATH.

Dec 20 2017, 7:19 PM · Restricted Project
rjmccall accepted D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared..

Okay, I think that's reasonable enough. LGTM.

Dec 20 2017, 7:16 PM

Dec 19 2017

rjmccall added a comment to D41394: [CodeGen] Support generation of TBAA info in the new format.

Rewriting some of the most basic tests would be fine. Please either use new FileCheck lines or clone the existing tests, since we don't really know how long this transition will last.

Dec 19 2017, 10:12 AM · Restricted Project
rjmccall added a comment to D41394: [CodeGen] Support generation of TBAA info in the new format.

Tests?

Dec 19 2017, 8:05 AM · Restricted Project

Dec 18 2017

rjmccall added inline comments to D41228: [ObjC] Enable __strong pointers in structs under ARC.
Dec 18 2017, 10:21 PM

Dec 15 2017

rjmccall added a comment to D41228: [ObjC] Enable __strong pointers in structs under ARC.

I accidentally hit 'send' too early on my review, so here's part two. I still haven't fully reviewed the new IRGen file.

Dec 15 2017, 8:14 PM
rjmccall added a comment to D41228: [ObjC] Enable __strong pointers in structs under ARC.

You should add a has_feature check for this (has_feature(objc_arc_fields)?), as well as documentation. The documentation would go in the ARC spec.

Dec 15 2017, 8:01 PM
rjmccall added inline comments to D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared..
Dec 15 2017, 7:00 PM
rjmccall accepted D41301: ASan+operator new[]: Fix operator new[] cookie poisoning.

LGTM.

Dec 15 2017, 6:51 PM
rjmccall added a comment to D36790: [ObjC] Messages to 'self' in class methods that return 'instancetype' should use the pointer to the class as the result type of the message.

LGTM outside of a comment request; please feel free to commit when you'd made that change.

Dec 15 2017, 6:50 PM