Page MenuHomePhabricator

erichkeane (Erich Keane)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 28 2016, 8:37 AM (238 w, 1 d)

Recent Activity

Yesterday

erichkeane committed rG8776e3f289c1: [EXTINT][OMP] Fix _ExtInt type checking in device code (authored by erichkeane).
[EXTINT][OMP] Fix _ExtInt type checking in device code
Wed, Jan 20, 11:36 AM

Thu, Jan 14

erichkeane committed rG9e53c94d8dd7: [NFC] Update test to not check for 'opaque' in the file name. (authored by erichkeane).
[NFC] Update test to not check for 'opaque' in the file name.
Thu, Jan 14, 11:24 AM

Mon, Jan 11

erichkeane accepted D94438: Fis for Assertion failure on dependent expression..

Fix looks fine, I'm on the fence about how to handle the test, whether it is valuable to convert it to a C++ test and omit the #ifdef/extra run line, or leave it as it is (without the 2nd run with Wmost).

Mon, Jan 11, 11:34 AM · Restricted Project

Thu, Jan 7

erichkeane added a comment to D92715: [Clang][RISCV] Define RISC-V V builtin types.

I think it would be possible to add a new attribute to define these types. But only specific values for parameters would be allowed so that it could only generate the exact types we see here and the future segment load patches. It wouldn't be general purpose like vector_size since the backend design has a contract about how to translate the LMUL value from the scalable type.

This would introduce a new type in the AST or at least new subtype of VectorType. This new type would need to be checked for in multiple places in the compiler to define its behavior. As a datapoint VectorType::SveFixedLengthDataVector appears 20 times. It's not clear that's a reduction in complexity versus following what was already done with AArch64SVEACLETypes.def. But I'm not a frontend expert.

Thu, Jan 7, 4:03 PM · Restricted Project
erichkeane committed rG43043adcfbc6: Add element-type to the Vector TypeLoc types. (authored by erichkeane).
Add element-type to the Vector TypeLoc types.
Thu, Jan 7, 9:15 AM
erichkeane closed D93483: Add element-type to the Vector TypeLoc types..
Thu, Jan 7, 9:14 AM · Restricted Project

Wed, Jan 6

erichkeane committed rG3fa6cedb6be8: Fix MaterializeTemporaryExpr's type when its an incomplete array. (authored by erichkeane).
Fix MaterializeTemporaryExpr's type when its an incomplete array.
Wed, Jan 6, 7:25 AM
erichkeane closed D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..
Wed, Jan 6, 7:25 AM · Restricted Project
erichkeane added a comment to D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

Wed, Jan 6, 6:15 AM · Restricted Project

Tue, Jan 5

erichkeane added inline comments to D93483: Add element-type to the Vector TypeLoc types..
Tue, Jan 5, 9:15 AM · Restricted Project
erichkeane accepted D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

The comment claims: "// Special case where the argument is a template id.". I would expect one of the following to hit that:
https://godbolt.org/z/znYW1s

I tried this snippet with the patch (and also an added assert(!Attr.isArgIdent(0)); to HandleVectorSizeAttr just to be sure). Compiled as expected, no crash.

Tue, Jan 5, 9:04 AM · Restricted Project
erichkeane added a comment to D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

I'm generally OK with this (the asserts are unnecessary), but would like @aaron.ballman to double check my expectations here.

It would be great if we could get confirmation!

I tried a few different things to construct matrix_type attributes with ArgIdents, but failed. The patch also adjusts the code for a bunch of attributes. So if there are indeed cases where ArgIdents can show up, we will get some examples for unit tests.

Tue, Jan 5, 8:56 AM · Restricted Project
erichkeane added a comment to D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).

Tue, Jan 5, 8:44 AM · Restricted Project
erichkeane added a comment to D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..

Ping!

Tue, Jan 5, 5:48 AM · Restricted Project
erichkeane added a comment to D93483: Add element-type to the Vector TypeLoc types..

Ping!

Tue, Jan 5, 5:48 AM · Restricted Project

Mon, Jan 4

erichkeane added a comment to D93597: [X86][SSE] Enable constexpr on some basic SSE intrinsics (RFC).
Mon, Jan 4, 12:47 PM · Restricted Project
erichkeane added a comment to D92523: Small improvements to Intrinsic::getName.

I note here: https://llvm.org/doxygen/namespacellvm_1_1Intrinsic.html#a7157f9fa9dd11f234ec3c58517cb6d96 that the documentatoin to the getName function variant says:

Mon, Jan 4, 10:16 AM · Restricted Project

Dec 17 2020

erichkeane requested review of D93483: Add element-type to the Vector TypeLoc types..
Dec 17 2020, 12:15 PM · Restricted Project

Dec 15 2020

erichkeane updated the diff for D51650: Implement target_clones multiversioning.

Since there seems to be at least a little renewed interest in this from a handful of people lately, I spend the time to rebase this and do some minor cleanup tasks.

Dec 15 2020, 6:36 AM
erichkeane added a comment to D93103: Enable the _ExtInt extension on the BPF Target.

From the CFE's perspective, this is all that should be needed. Is there a BPF code-owner who can test it and see if it is acceptable?

Dec 15 2020, 6:04 AM · Restricted Project

Dec 7 2020

erichkeane added a comment to D92721: [PATCH] [clang] Create SPIRABIInfo to enable SPIR_FUNC calling convention.

From my perspective I think this is right... I don't feel comfortable approving this however until someone from OpenCL/OpenMP takes a look.

Dec 7 2020, 12:42 PM · Restricted Project
erichkeane committed rG1c98f984105e: Stop ExtractTypeForDeductionGuide from recursing on TypeSourceInfo (authored by erichkeane).
Stop ExtractTypeForDeductionGuide from recursing on TypeSourceInfo
Dec 7 2020, 11:30 AM
erichkeane accepted D92222: [Sema] Make more overload candidate types use iterator_ranges (NFC).
Dec 7 2020, 6:28 AM · Restricted Project
erichkeane added a comment to D51650: Implement target_clones multiversioning.

Fix @rsmith s comments, rebase on the big CPUDispatch refactor.

Ping. What's the status here?

Dec 7 2020, 6:24 AM

Nov 17 2020

erichkeane committed rGa72f11ee20fe: Fix a pair of tests that would fail on a win32 box (authored by erichkeane).
Fix a pair of tests that would fail on a win32 box
Nov 17 2020, 2:30 PM
erichkeane committed rG6976fef05b7e: Update 'note-candiate' functions to skip lambda-conversion-op-overloads (authored by erichkeane).
Update 'note-candiate' functions to skip lambda-conversion-op-overloads
Nov 17 2020, 5:50 AM

Nov 5 2020

erichkeane committed rG6b104ea4b463: Implement Lambda Conversion Operators for All CCs for MSVC. (authored by erichkeane).
Implement Lambda Conversion Operators for All CCs for MSVC.
Nov 5 2020, 7:26 AM
erichkeane closed D90634: Implement Lambda Conversion Operators for All CCs for MSVC..
Nov 5 2020, 7:26 AM · Restricted Project

Nov 4 2020

erichkeane abandoned D41952: Clang test changes for 'no-overflow' flag for sdiv\udiv instructions.
Nov 4 2020, 9:43 AM
erichkeane added a comment to D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..

Ping! This was requested in post-commit on the last patch, so I'm hoping it shouldn't be too controversial.

Nov 4 2020, 9:42 AM · Restricted Project
erichkeane accepted D79279: Add overloaded versions of builtin mem* functions.

I went through all the comments here, plus looked at the code myself. I believe all of the comments by other reviewers have been fixed/answered acceptably. I don't have any additional comments to add, therefore I think it is appropriate to accept this revision.

Nov 4 2020, 9:34 AM · Restricted Project, Restricted Project
erichkeane updated the diff for D90634: Implement Lambda Conversion Operators for All CCs for MSVC..

Added test for +lambda as @rsmith requested.

Nov 4 2020, 6:24 AM · Restricted Project

Nov 3 2020

erichkeane added inline comments to D90634: Implement Lambda Conversion Operators for All CCs for MSVC..
Nov 3 2020, 5:43 PM · Restricted Project
erichkeane added a comment to D90634: Implement Lambda Conversion Operators for All CCs for MSVC..

This LGTM but you should wait a day or so in case @rjmccall has opinions.

Nov 3 2020, 9:00 AM · Restricted Project
erichkeane updated the diff for D90634: Implement Lambda Conversion Operators for All CCs for MSVC..

Made another attempt at fixing the comment. Wordsmithing welcomed/encouraged :)

Nov 3 2020, 8:43 AM · Restricted Project
erichkeane updated the diff for D90634: Implement Lambda Conversion Operators for All CCs for MSVC..

update comment as @aaron.ballman requested.

Nov 3 2020, 5:58 AM · Restricted Project

Nov 2 2020

erichkeane requested review of D90634: Implement Lambda Conversion Operators for All CCs for MSVC..
Nov 2 2020, 11:11 AM · Restricted Project

Oct 30 2020

erichkeane committed rGec809e4cfe0b: PR47372: Fix Lambda invoker calling conventions (authored by erichkeane).
PR47372: Fix Lambda invoker calling conventions
Oct 30 2020, 6:40 AM
erichkeane closed D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 30 2020, 6:40 AM · Restricted Project

Oct 29 2020

erichkeane updated the diff for D89559: PR47372: Fix Lambda invoker calling conventions.

@rjmccall : Think I got everything, is this what you mean?

Oct 29 2020, 1:57 PM · Restricted Project
erichkeane added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 29 2020, 1:23 PM · Restricted Project
erichkeane added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 29 2020, 1:07 PM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Ping!

Oct 29 2020, 6:58 AM · Restricted Project

Oct 26 2020

erichkeane accepted D90073: [Clang][CodeGen] fix failed assertion.

Please make sure the commit message is significantly more descriptive here than what is on the review.

Oct 26 2020, 10:43 AM · Restricted Project

Oct 23 2020

erichkeane updated the diff for D89559: PR47372: Fix Lambda invoker calling conventions.

Alright, I have the multi-emit dealt with, and the problems that come with that solved. This doesn't do the MSVC-emit-for-all-CCs thing, but I intend to do that in a separate patch with the same infrastructure.

Oct 23 2020, 1:37 PM · Restricted Project
erichkeane added inline comments to D90073: [Clang][CodeGen] fix failed assertion.
Oct 23 2020, 1:32 PM · Restricted Project

Oct 22 2020

erichkeane updated subscribers of D89559: PR47372: Fix Lambda invoker calling conventions.

Turns out this patch: https://github.com/llvm/llvm-project/commit/2e1e0491b7098fcfe01945e8f62cafe1fcb3cf36 is my problem. The issue has to do deducing a 'local' type in the return type. As a result, we choose to mangle any type 'containing' auto as 'auto'.

Oct 22 2020, 11:02 AM · Restricted Project
erichkeane added a comment to D88645: [Annotation] Allows annotation to carry some additional constant arguments..

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Oct 22 2020, 9:45 AM · Restricted Project, Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.

Separately, the MSVC mangler should support Clang's CCs if there's a reasonable extensible rule there. I've never been able to figure out the MVSC mangling grammar.

Same here :)

Oct 22 2020, 9:18 AM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Hmm... so I got distracted the last few days with a handful of small SEMA issues that I believe to be solved, so I'm going back to my codegen issues.

Oct 22 2020, 9:00 AM · Restricted Project

Oct 19 2020

erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Since the work is 99% the same, I think I should start doing that (emitting BOTH in the case where it matches the default member but NOT the default free function CC) in this review, then do a separate commit for the MSVC compat mode where we emit a version for ALL of the calling conventions.

You'll also need an overload resolution tiebreaker to ensure that +[]{} and the like are not ill-formed due to ambiguity.

Oct 19 2020, 5:24 PM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.

Oct 19 2020, 10:08 AM · Restricted Project
erichkeane added a comment to D86936: [clang] Limit the maximum level of fold-expr expansion..

Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:

Sets the limit for nested parentheses, brackets, and braces to N in blocks, declarators, expressions, and struct or union declarations.

The 256 default limit is REALLY small for a fold expression, particularly since the instantiation depth limit is 1024 by default. I think this patch needs to be changed to use the InstantiationDepth instead. @rjmccall, thoughts?

Oct 19 2020, 6:32 AM · Restricted Project
erichkeane updated subscribers of D86936: [clang] Limit the maximum level of fold-expr expansion..

Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:

Oct 19 2020, 6:27 AM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.

I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's at least more conformant than that.

Oct 19 2020, 5:51 AM · Restricted Project

Oct 16 2020

erichkeane added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 16 2020, 4:49 PM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

Perhaps a better example:

Oct 16 2020, 11:56 AM · Restricted Project
erichkeane added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.

Is the AttributedType still present in CallOperator->getType()?

Oct 16 2020, 11:55 AM · Restricted Project
erichkeane updated the diff for D89559: PR47372: Fix Lambda invoker calling conventions.

Calc->get.

Oct 16 2020, 10:35 AM · Restricted Project
erichkeane added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 16 2020, 10:34 AM · Restricted Project
erichkeane added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 16 2020, 10:17 AM · Restricted Project
erichkeane requested review of D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 16 2020, 9:13 AM · Restricted Project

Oct 13 2020

erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

Hmm... I'll have to consider that. That is an interesting thought. Presumably we could just copy the 'getLambdaManglingNumber' stuff in that case, and place the value in the same location in the mangling, with some sort of discriminator (to avoid conflicting manglings).

The Itanium mangling already produces a different lambda mangling number for different lambda signatures. You just need the kernel-ness to be part of the signature.

It does it via the getLambdaManglingNumber I believe, right? The idea would be to have a getKernelLambdaManglingNumber that does something similar, and needs something to avoid conflicting, which is the discriminator I was mentioning.

Right, so if you look how that's ultimately derived in ItaniumNumberingContext::getManglingNumber, you'll see that every context has different sequences for unique lambda-sigs. If you can just put "this is a kernel" into the lambda-sig, you'll automatically get a stable sequence for kernel lambdas. But that would rely on immediately knowing that a lambda will be used as a kernel.

The only other concern I have is WHEN we know that a lambda is used in a kernel, which we don't until it is called (and can cause confusion when it is a template parameter and called later).

Oh, yes, if you don't know that a lambda is used as a kernel locally then this falls apart a bit. You could of course pretend for ABI purposes that there's a new intermediate lambda at the kernel use point when the lambda is not local to the current function. I don't think there's anything which relies on mangling lambdas before the function they're contained within is fully type-checked.

One thing that might save us from this, is we aren't actually modifying the LAMBDAs mangling, we're modifying the KERNEL's mangling. The lambda itself keeps its mangling, but the kernel that calls it (which is the device/host boundary) is what has its name changed. So at least at that point we know some about it. And, actually, since we are generating the 'integeration header' lookup table AND the kernel in identical (if not the same) invocation, perhaps a global counter WOULD work... My coworker is investigating this now, so hopefully she'll be able to prove out this idea and give further feedback here.

I feel obliged to point out that, if you're worried about a single function potentially having a different set of lambdas in different translation modes, that's also going to affect things like template specialization identity. That is, suppose a kernel function is formed for a particular lambda, and in one translation mode it's the first lambda with that signature in a function and in another mode it's the second. That's going to change the immediate mangling of the lambda, but it's also going to change the mangling of templates used with that lambda. For example, if you have

Oct 13 2020, 11:44 AM · Restricted Project
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

Hmm... I'll have to consider that. That is an interesting thought. Presumably we could just copy the 'getLambdaManglingNumber' stuff in that case, and place the value in the same location in the mangling, with some sort of discriminator (to avoid conflicting manglings).

The Itanium mangling already produces a different lambda mangling number for different lambda signatures. You just need the kernel-ness to be part of the signature.

Oct 13 2020, 10:04 AM · Restricted Project
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Coming up with said stable sequence is somewhat difficult as well. A strict integer ordering didn't end up being stable, as some of the kernel handling can cause instantiations to happen in a slightly different ordering, which messed that up, as would use of the builtin. We wanted to find a way that was dependent on the source code alone. The "Quick and Dirty" solution was line/column numbers, though we considered a hash of that same information to at least make it shorter.

Certainly you wouldn't want a *global* sequence ID. However, lambdas can't just occur globally, they're always part of some declaration that they can be scoped to, so that you have e.g. "the third kernel lambda within function X". I fail to see how that wouldn't address the concern about instantiation order, and it's still source-directed.

Oct 13 2020, 9:49 AM · Restricted Project
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence?

Oct 13 2020, 9:19 AM · Restricted Project
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

CUDA/HIP are facing similar issues, i.e. consistency of name mangling of kernels between host/device compilation of the same TU. I hope this feature to be implemented in a generic way so that it may be reusable for other offloading languages.

Oct 13 2020, 7:24 AM · Restricted Project
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

The feature that this supports is a part of the SYCL 2020 Provisional Spec, I thought that was sufficient. We'll look into an RFC to re-submit in the future.

Does that cover only functionality that can be implemented in terms of this builtin, or the builtin itself? If so, what is the functionality that needs to be supported? That information will be a good starting point for the RFC.

Oct 13 2020, 5:58 AM · Restricted Project

Oct 12 2020

erichkeane committed rGac73cafac0e5: Ensure TreeTransform considers ParmVarDecls as transformed Decls (authored by erichkeane).
Ensure TreeTransform considers ParmVarDecls as transformed Decls
Oct 12 2020, 2:38 PM
erichkeane added a comment to D76620: [SYCL] Implement __builtin_unique_stable_name..

Richard and I just found out about this commit, and we've decided to revert it. I apologize for the very late reversion, but the reality of Clang development is that it's very difficult for the code owners to watch literally every commit, and we rely on the community to follow policies and bring things to our attention when there's a question about the right thing to do. This is a new feature that was submitted without properly following any of the guidelines for new features: it was neither approved by a relevant standards body, added for compatibility with an existing frontend, or taken through the RFC process. If you had brought it to our attention as an RFC, we would've expressed serious concerns about the design.

Please start an RFC thread and CC Richard and me.

Oct 12 2020, 6:18 AM · Restricted Project

Oct 8 2020

erichkeane added a comment to D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..

*PING!* Any feedback?

Oct 8 2020, 10:07 AM · Restricted Project

Sep 29 2020

erichkeane resigned from D88430: [OpenMP] Replace OpenMP RTL Functions With OMPIRBuilder and OMPKinds.def.

I'm not really a good person to review this, OMP isn't nearly my expertise.

Sep 29 2020, 10:28 AM · Restricted Project, Restricted Project, Restricted Project

Sep 25 2020

erichkeane added inline comments to D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..
Sep 25 2020, 6:12 AM · Restricted Project
erichkeane requested review of D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..
Sep 25 2020, 6:11 AM · Restricted Project

Sep 24 2020

erichkeane committed rGf8a92adfa242: Remove dead branch identified by @rsmith on post-commit for D88236 (authored by erichkeane).
Remove dead branch identified by @rsmith on post-commit for D88236
Sep 24 2020, 1:05 PM
erichkeane added a comment to D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

Ok then, I'll take a look to see what it would take to make the MaterializeTemporaryExpr be created with the complete type here instead. Thanks!

Sep 24 2020, 12:27 PM
erichkeane closed D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

Gah, forgot 'Differential Revision' again :/

Sep 24 2020, 12:10 PM
erichkeane committed rG606a734755d1: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes (authored by erichkeane).
[PR47636] Fix tryEmitPrivate to handle non-constantarraytypes
Sep 24 2020, 12:10 PM
erichkeane updated the diff for D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

@rjmccall Done!

Sep 24 2020, 11:35 AM
erichkeane added a comment to D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.

Note: @rsmith and @rjmccall were the last ones in this section based on Blame (by a wide margin!), so added both as reviewers.

Sep 24 2020, 8:57 AM
erichkeane requested review of D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes.
Sep 24 2020, 8:57 AM

Aug 26 2020

erichkeane accepted D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization.
Aug 26 2020, 12:02 PM · Restricted Project
erichkeane added inline comments to D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization.
Aug 26 2020, 11:30 AM · Restricted Project

Aug 21 2020

erichkeane accepted D86339: Enable constexpr on BITREVERSE builtin intrinsics (PR47249).

This looks right to me.

Aug 21 2020, 10:59 AM · Restricted Project
erichkeane accepted D86342: Enable constexpr on ROTATELEFT/ROTATERIGHT builtin intrinsics (PR47249).

Its a bit of a shame that the two implementations (left/right) differ by only a single character, an y ideas for combining them cleanly? I might also consider combining the two 'if' statements into a single one (if !EvalInteger(E->getArg(0)...) || !EvalInteger(E->getArg(1)...), but that is generally just preference.

Aug 21 2020, 10:43 AM · Restricted Project

Aug 19 2020

erichkeane added inline comments to D86229: [X86] Enable constexpr on POPCNT intrinsics (PR31446).
Aug 19 2020, 12:11 PM · Restricted Project
erichkeane accepted D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types.
Aug 19 2020, 10:10 AM · Restricted Project
erichkeane accepted D86187: [X86] Add support 'tune' in target attribute.

CFE changes look right to me. Please give the rest of the reviews a little while to take a look though

Aug 19 2020, 5:57 AM · Restricted Project

Aug 18 2020

erichkeane added a comment to D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types.

Were you able to make any progress on this?

Aug 18 2020, 4:01 PM · Restricted Project

Aug 17 2020

erichkeane added inline comments to D85384: [X86] Add basic support for -mtune command line option in clang.
Aug 17 2020, 12:32 PM · Restricted Project
erichkeane added a comment to D85384: [X86] Add basic support for -mtune command line option in clang.

1 comment, otherwise seems alright to me.

Aug 17 2020, 12:25 PM · Restricted Project

Aug 14 2020

erichkeane added inline comments to D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types.
Aug 14 2020, 12:46 PM · Restricted Project
erichkeane added a comment to D74361: [Clang] Undef attribute for global variables.

Yep! Declaring a global variable that isn't 'extern' with an incomplete type is disallowed anyway, so if you call RequireCompleteType, you're likely just diagnosing that early.

Aug 14 2020, 8:53 AM · Restricted Project
erichkeane added a comment to D74361: [Clang] Undef attribute for global variables.

I did a little debugging, and the problem is the template itself isn't a complete type until you require it with RequireCompleteType. You have this same problem with incomplete types: https://godbolt.org/z/hvf3M4

Aug 14 2020, 8:12 AM · Restricted Project
erichkeane added a comment to D74361: [Clang] Undef attribute for global variables.

Also, see this bug: This crashes immediately when used on a template instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169

Aug 14 2020, 7:05 AM · Restricted Project
erichkeane added a comment to D74361: [Clang] Undef attribute for global variables.

This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt

Please take a look, and if it takes some time please revert while you investigate.

Thanks! It seems Windows inserts 'dso_local' into the middle of the generated IR.

I can't test on Windows so the two that failed CI are now marked as "UNSUPPORTED: system-windows".

Do you know a usual work around for variation in symbol visibility? I'm happy to copy it from another test but am wary of guessing what might work on Windows.

Aug 14 2020, 7:04 AM · Restricted Project

Aug 12 2020

erichkeane committed rGaa4bc1cb7978: Limit Max Vector alignment on COFF targets to 8192. (authored by erichkeane).
Limit Max Vector alignment on COFF targets to 8192.
Aug 12 2020, 6:36 AM
erichkeane closed D85543: Limit Max Vector alignment on COFF targets to 8192.
Aug 12 2020, 6:35 AM · Restricted Project

Aug 10 2020

erichkeane added a reviewer for D85543: Limit Max Vector alignment on COFF targets to 8192: riccibruno.
Aug 10 2020, 3:41 PM · Restricted Project

Aug 7 2020

erichkeane requested review of D85543: Limit Max Vector alignment on COFF targets to 8192.
Aug 7 2020, 12:11 PM · Restricted Project