Page MenuHomePhabricator

erichkeane (Erich Keane)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

erichkeane added a comment to D116203: [clang] adds unary type transformations as compiler built-ins.

No comments other than the ones others have made, though the make_signed/make_unsigned needs to work for _BitInt

Wed, Jan 19, 9:24 AM · Restricted Project
erichkeane added a comment to D117611: [Sema] Warn about printf %n on Android and Fuchsia.

Other than the clang-format issues, this looks fine. However, I'm not really knowledgable enough/the owner of the Fuschia/Android stuff, so I'd prefer an approval 'in concept' come from someone more involved with that.

Wed, Jan 19, 6:24 AM

Yesterday

erichkeane accepted D117560: [C++20][Concepts] Fix a failed assertion on an invalid typename requirement.
Tue, Jan 18, 7:00 AM · Restricted Project
erichkeane committed rG67ac3f1fbeec: [Driver] Pass the flag -dI to cc1 invocation (authored by qichaogu).
[Driver] Pass the flag -dI to cc1 invocation
Tue, Jan 18, 6:17 AM
erichkeane closed D117292: [Driver] Pass the flag -dI to cc1 invocation.
Tue, Jan 18, 6:17 AM · Restricted Project

Fri, Jan 14

erichkeane committed rG2bcba21c8ba9: [CPU-Dispatch] Make sure Dispatch names get updated if previously mangled (authored by erichkeane).
[CPU-Dispatch] Make sure Dispatch names get updated if previously mangled
Fri, Jan 14, 10:46 AM
erichkeane added a comment to D114235: [clang] Extend ParsedAttr to allow custom handling for type attributes.

Right, yeah, so there are a couple of problems with AttributedType. First, it gets lost almost as soon as you get out of SemaType about 90% of the time. Anything that does some level of canonicalization ends up losing it, so the AttributedType information is lost almost immediately. This is why the current ones all store information in the ExtInfo. The worst place for this ends up being in the template instantiator, which immediately canonicalizes/desugars types all over the place.

Fri, Jan 14, 9:16 AM · Restricted Project
erichkeane accepted D117292: [Driver] Pass the flag -dI to cc1 invocation.
Fri, Jan 14, 5:53 AM · Restricted Project

Thu, Jan 13

erichkeane added inline comments to D117238: [C2x] Add BITINT_MAXWIDTH support.
Thu, Jan 13, 10:50 AM · Restricted Project
erichkeane added inline comments to D117238: [C2x] Add BITINT_MAXWIDTH support.
Thu, Jan 13, 10:44 AM · Restricted Project
erichkeane accepted D117238: [C2x] Add BITINT_MAXWIDTH support.

CFE changes LGTM, the limits.h/.cpp changes LOOK right, but please give others a chance to take a look.

Thu, Jan 13, 10:41 AM · Restricted Project
erichkeane committed rGb699e8b11aa9: Add another assert to cpu-dispatch emission to help track down a tough (authored by erichkeane).
Add another assert to cpu-dispatch emission to help track down a tough
Thu, Jan 13, 6:54 AM

Wed, Jan 12

erichkeane committed rG6e77ad11ffab: Add an assert in cpudispatch emit to try to track down an error. (authored by erichkeane).
Add an assert in cpudispatch emit to try to track down an error.
Wed, Jan 12, 10:32 AM

Mon, Jan 10

erichkeane added a comment to D98895: [X86][clang] Disable long double type for -mno-x87 option.

Linus Torvalds' thoughts on the patches as a result of this change.

Those uses of 1E6L were perhaps strange, but they were only used in
constant compile-time expressions that were cast to 'unsigned long' in
the end, so how the heck did llvm end up with any floating point in
there?

In this case there was really no excuse not to just use a better
constant, but there are other situations where it might well be quite
reasonable to use floating point calculations to create an integer
constant (eg maybe some spec describes an algorithm in FP, but the
implementation uses fixed-point arithmetic and has initializers that
do the conversion).

Imagine for a moment that you want to do fixed-point math (perhaps
because you have a microcontroller without an FP unit - it's not
limited to just "the kernel doesn't do FP"). Further imagine just for
a moment that you still want some fundamental constants like PI in
that fixed-point format.

The sane way to generate said constants is to do something like

#define FIXPT_1 (1u << FIXPT_SHIFT)
#define FIXPT_FP(x) ((fixpt_t) (((x)*FIXPT_1)+0.5))
#define FIXPT_PI FIXPT_FP(3.14159265)

rather than have to do something incredibly stupid and illogical and
unreadable like

#define FIXPT_PI 205887

So honestly, this seems to be just llvm being completely stupid. The
fact that you don't want to generate floating point code has *NOTHING*
to do with floating point literals for constant expressions.

In fact, even if you don't want to generate floating point code -
again, maybe you don't have a FP unit - doesn't mean that you might
not want to generate normal floating point constants. You may end up
having explicit software floating point, and doing things like passing
the floating point values around manually, ie

 union fp {
     uint64_t val;
     double fp_val;
};

and having code like

static const union fp sqrt2 = { .fp_val = 1.4142.... };

and then doing all the math in 'uint64_t' just because you wrote a
couple of math routines yourself:

fp_mul(&res,  sqrt2.val, x);

again, there's no floating point *code* generated, but that doesn't
mean that the compiler shouldn't allow users to use floating point
*values*.

The two
examples above are very much not about the Linux kernel (although I
think we actually have a couple of places that do things exactly like
that) they are about generic coding issues.

Mon, Jan 10, 12:04 PM · Restricted Project
erichkeane added a comment to D116736: [Clang] Add __builtin_reduce_or and __builtin_reduce_and.

LGTM other than the NIT found by @fhahn

Mon, Jan 10, 6:26 AM · Restricted Project
erichkeane accepted D116792: [AST] lookup in parent DeclContext for transparent DeclContext.

I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb

I seem to remember hitting this assert, and from my end, I think I decided even calling 'lookup' with the linkage spec to be a mistake (BTW, you might consider updating that 'Encloses' for 'export' as well!).

Yeah, it is another bug for 'export'. I've tried to address it in https://reviews.llvm.org/D116911 with the same style.

Is there any mechanism in the parent call of 'lookup' here to make it get the right thing?

And 'lookup' is called in various places. For example, from the stack trace of the crash, we could find that the parent of call is DeclareImplicitDeductionGuides. And I tried to handle it in DeclareImplicitDeductionGuides, then the compiler would crash again at LookupDirect in SemaLookup.cpp. I feel it is not good to add checks for places to call lookup. I agree that it is odd to lookup in a transparent DeclContext. But I feel it is not bad to lookup in the enclosing context from the definition of transparent DeclContext. Any thoughts?

Mon, Jan 10, 6:17 AM · Restricted Project
erichkeane accepted D116911: [AST] Don't consider 'ExportDecl' when calculating DeclContext 'Encloses'.
Mon, Jan 10, 6:15 AM · Restricted Project

Fri, Jan 7

erichkeane added a comment to D116792: [AST] lookup in parent DeclContext for transparent DeclContext.

I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb

Fri, Jan 7, 6:21 AM · Restricted Project

Thu, Jan 6

erichkeane added a comment to D114425: [clang] Add __builtin_bswap128.

I kind of wonder if we should detect the __int128 type being requested in ASTContext::GetBuiltinType and return an error up to Sema::LazilyCreateBuiltin. Probably requires a new error code and handling for it in LazilyCreateBuiltin. I assume that would catch the bad builtin earlier. As it stands now we'd still allow it if it constant folds away in ExprConstant.cpp so that it never reaches CGBuiltin.cpp. But I'm not a frontend expert. Adding more potential reviewers

Thu, Jan 6, 12:07 PM · Restricted Project

Tue, Jan 4

erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Should be fixed here: 2edc21e8566be8fa9b20e0bb71a83af90ec9aa97

Tue, Jan 4, 9:53 AM · Restricted Project
erichkeane committed rG2edc21e8566b: Fix altivec regression caused by D115670 in Vec Const Eval (authored by erichkeane).
Fix altivec regression caused by D115670 in Vec Const Eval
Tue, Jan 4, 9:53 AM
erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Hi!
Sorry for the delay, I just got back from my christmas break. I'll take a look later today/this week.

Tue, Jan 4, 7:58 AM · Restricted Project

Dec 17 2021

erichkeane committed rGa94f68a2bd31: Implement some constexpr vector unary operators, fix boolean-ops (authored by erichkeane).
Implement some constexpr vector unary operators, fix boolean-ops
Dec 17 2021, 6:09 AM
erichkeane closed D115670: Implement some constexpr vector unary operators, fix boolean-ops.
Dec 17 2021, 6:08 AM · Restricted Project

Dec 16 2021

erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Note: I think I've done everything requested, so I'm hoping to commit this tomorrow 1st thing in order to have it in time for everyone's vacations (and so my downstream can get it before the new year), unless I hear objections from the other reviewers.

Dec 16 2021, 9:55 AM · Restricted Project

Dec 15 2021

erichkeane updated the diff for D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Fix Aaron+ Craig's comments.

Dec 15 2021, 10:47 AM · Restricted Project
erichkeane updated the diff for D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Implement the other two easily doable operators, do Craigs suggestions.

Dec 15 2021, 10:08 AM · Restricted Project
erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

@craig.topper : Yep, probably so in both those cases. I'm working on a patch that includes the 2 'nots' as well, which complicates this whole section a bunch though.

Dec 15 2021, 9:55 AM · Restricted Project
erichkeane added inline comments to D115670: Implement some constexpr vector unary operators, fix boolean-ops.
Dec 15 2021, 8:15 AM · Restricted Project
erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Note, my pre-merge check failure seems completely unrelated. It seems to have some problem with the some 'go' bindings, but I don't believe that has anything to do with this change.

Dec 15 2021, 7:33 AM · Restricted Project

Dec 14 2021

erichkeane accepted D115253: [C2x] Support the *_WIDTH macros in limits.h and stdint.h.
Dec 14 2021, 6:29 AM · Restricted Project
erichkeane updated the diff for D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Enable __int128 vectors, and fix the issue of the >64 bit vectors for negation. Also add a test for these.

Dec 14 2021, 6:24 AM · Restricted Project

Dec 13 2021

erichkeane added inline comments to D115670: Implement some constexpr vector unary operators, fix boolean-ops.
Dec 13 2021, 3:55 PM · Restricted Project
erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

To add:
I DID just try to fix that thing:

Dec 13 2021, 1:56 PM · Restricted Project
erichkeane added inline comments to D115670: Implement some constexpr vector unary operators, fix boolean-ops.
Dec 13 2021, 1:39 PM · Restricted Project
erichkeane added a comment to D115670: Implement some constexpr vector unary operators, fix boolean-ops.

Note I am adding the folks who were added as reviewers the last time I did vector constexpr work: https://reviews.llvm.org/D79755

Dec 13 2021, 1:23 PM · Restricted Project
erichkeane requested review of D115670: Implement some constexpr vector unary operators, fix boolean-ops.
Dec 13 2021, 1:21 PM · Restricted Project

Dec 10 2021

erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

ping?

@erichkeane Since you are pushing for upgrade the gcc/clang requirement as well, would you take care of that?

Dec 10 2021, 6:18 AM · Restricted Project, Restricted Project

Dec 9 2021

erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 9 2021, 6:45 AM · Restricted Project
erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 9 2021, 6:07 AM · Restricted Project
erichkeane accepted D115441: [X86][MS] Add 80bit long double support for Windows.

This looks good to me, and mirrors something I implemented in our downstream a few years ago. Please don't submit for another day or two to give others a chance to review.

Dec 9 2021, 5:58 AM · Restricted Project, Restricted Project

Dec 8 2021

erichkeane added a comment to D115337: [X86][clang] Put the update of HasLongDouble into TargetInfo::adjust.

@craig.topper is the only one I know who can keep all of this straight, so perhaps he's willing to give a more in depth review?

Dec 8 2021, 6:37 AM · Restricted Project
erichkeane added a reviewer for D115337: [X86][clang] Put the update of HasLongDouble into TargetInfo::adjust: craig.topper.
Dec 8 2021, 6:36 AM · Restricted Project
erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 8 2021, 6:09 AM · Restricted Project

Dec 7 2021

erichkeane added a comment to D112616: Fix crash on invalid code involving late parsed inline methods.

The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST.

Are deduction guides just not normally allowed for methods?

Also, while I don't think it would eliminate the need to be defensive here, our recovery from misnamed constructors is pretty bad, and it would be great if we recognized that pattern in the parser.

Dec 7 2021, 11:35 AM · Restricted Project
erichkeane accepted D115228: [Clang] Do not check if we are in a discared context in non-immediate contexts.
Dec 7 2021, 9:00 AM · Restricted Project
erichkeane accepted D112616: Fix crash on invalid code involving late parsed inline methods.

I'm about as confident as _I_ could be on this one, so between that and the time, I think my LGTM now carries a bit of weight.

Dec 7 2021, 8:58 AM · Restricted Project
erichkeane added inline comments to D115253: [C2x] Support the *_WIDTH macros in limits.h and stdint.h.
Dec 7 2021, 8:43 AM · Restricted Project
erichkeane added a comment to D115253: [C2x] Support the *_WIDTH macros in limits.h and stdint.h.

A few nits, but not nearly 'expert' enough to approve without giving everyone else some time to look this over.

Dec 7 2021, 8:26 AM · Restricted Project
erichkeane added a comment to D115248: [clang] Fix PR28101.

Can you add some tests please? Additionally, we don't typically use LLVM_UNLIKELY like you did there.

Dec 7 2021, 7:41 AM · Restricted Project
erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 7 2021, 5:56 AM · Restricted Project

Dec 6 2021

erichkeane accepted D108643: Introduce _BitInt, deprecate _ExtInt.
Dec 6 2021, 9:59 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 6 2021, 8:51 AM · Restricted Project
erichkeane accepted D115141: [ARM][clang] Add back branch protection tests.

I'm quite sure this is the right fix, the 'requires' clause missing is, I believe, the problems that caused us to revert this test.

Dec 6 2021, 6:22 AM · Restricted Project

Dec 2 2021

erichkeane added inline comments to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.
Dec 2 2021, 8:40 AM · Restricted Project

Dec 1 2021

erichkeane added a comment to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.

I think the safest bet is to be conservative and not allow mixing packs with variadics, and not allowing multiple packs. We should be able to diagnose that situation from ClangAttrEmitter.cpp to help attribute authors out. However, it'd be worth adding a FIXME comment to that diagnostic logic asking whether we want to relax the behavior at some point. But if you feel strongly that we should support these situations initially, I wouldn't be opposed.

Dec 1 2021, 11:43 AM · Restricted Project
erichkeane added a comment to D112421: [clang][ARM] PACBTI-M frontend support.

I just found that build failure in our downstream, and did a little investigation. Running the clang-invocations directly resulted in :

Dec 1 2021, 8:56 AM · Restricted Project, Restricted Project
erichkeane added a comment to D114439: [Annotation] Allow parameter pack expansions in annotate attribute.

Aaron is way more familiar with this code than I am, but I've got some suggestions for more tests in the parsing, we need to make sure that we handle pack expansion completely here.

Dec 1 2021, 8:50 AM · Restricted Project

Nov 30 2021

erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

Right, but last time we did the motivation was specifically to get to c++14, while here the motivation is to drop an old MSVC according to the MSVC-specific support we intend to provide.

My memory is that that was _A_ motivation, not the only one. There were quite a few GCC bugs that we were getting away from as well that was my primary justification for pushing it at the time, though the C++14 motivation was ALSO tempting/appreciated.

I'm fairly sure that at least JF who pushed for it was motivated by C++14, here is the RFC: https://lists.llvm.org/pipermail/llvm-dev/2019-January/129452.html
The update of toolchain has always historically been measured by the amount of features we get from the update, in particular while compilers were getting support for new standard feature during C++11 adoption it was really a game of matching the various compiler, looks at potential updates and what this would enable. Though I agree that we're somehow frequently working around issues specific to gcc-5 (I'd say my gcc-5.4 bot breaks once a week on average), and migrating from old toolchains can have value on its own.

Nov 30 2021, 11:17 AM · Restricted Project, Restricted Project
erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

IMO, if we're updating the MSVC versions, we should do the same for the GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and Clang 3.5 is from 2014.

We've always handled MSVC update separately I believe, we can't just take the "last two version of MSVC" guideline and update every compiler each time.

My understanding is this would only be the ~3rd time we've done this. The last time we did this, we did them all together.

Right, but last time we did the motivation was specifically to get to c++14, while here the motivation is to drop an old MSVC according to the MSVC-specific support we intend to provide.

Nov 30 2021, 11:03 AM · Restricted Project, Restricted Project
erichkeane updated subscribers of D114639: Raise the minimum Visual Studio version to VS2019.

IMO, if we're updating the MSVC versions, we should do the same for the GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and Clang 3.5 is from 2014.

We've always handled MSVC update separately I believe, we can't just take the "last two version of MSVC" guideline and update every compiler each time.

Nov 30 2021, 10:28 AM · Restricted Project, Restricted Project
erichkeane added inline comments to D112663: [clang-repl] Allow Interpreter::getSymbolAddress to take a mangled name..
Nov 30 2021, 8:58 AM
erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

See: https://lists.llvm.org/pipermail/llvm-dev/2021-November/153882.html for what I was thinking of.

Nov 30 2021, 6:36 AM · Restricted Project, Restricted Project
erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

IMO, if we're updating the MSVC versions, we should do the same for the GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and Clang 3.5 is from 2014.

I'd be in favor of that; we could even see if we're ready to allow C++17 in the code base (which would be a pretty nice win for if init statements alone). However, it's also a bit orthogonal to this particular patch. Do you (or someone else) want to start an RFC?

Nov 30 2021, 6:29 AM · Restricted Project, Restricted Project
erichkeane added a comment to D114639: Raise the minimum Visual Studio version to VS2019.

IMO, if we're updating the MSVC versions, we should do the same for the GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and Clang 3.5 is from 2014.

Nov 30 2021, 5:59 AM · Restricted Project, Restricted Project

Nov 29 2021

erichkeane accepted D114615: [NFC][clang]Increase the number of driver diagnostics.

Please ignore the clang-format suggestion.

Nov 29 2021, 9:01 AM · Restricted Project
erichkeane added a comment to D114080: [SYCL] Diagnose uses of zero length arrays.

I note that this is missing a test, otherwise I don't see any issues with it from my end.

Nov 29 2021, 7:26 AM · Restricted Project
erichkeane added a comment to D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default.

Either this or D108453 (which were committed together!) caused this assert according to my git-bisect: https://godbolt.org/z/4rqYKfW7K

NOTE that this fails in a lit-test for me, clang CodeGen/ifunc.c (though my downstream seems to run the verifier even with -emit-llvm, so you might need to just swap it to an -emit-obj to get this to repro).

The lit-test failure of CodeGen/ifunc.c was not directly related to this patch.
emitIFuncDefinition was creating an incorrect function attribute.
It added the noundef attribute to the function even though there are no parameters (foo_ifunc function of ifunc.c), and it was fixed a few days ago.

The patch that solved this problem is D113352.

The emitIFuncDefinition fucntion incorrectly passes the GlobalDecl of the IFunc itself to the call to GetOrCreateLLVMFunction for creating the resolver, which causes it to be created with a wrong attribute list, which fails Verifier::visitFunction with "Attribute after last parameter!". You'll note that unlike the relationship between aliases and their aliasees, the resolver and the ifunc have different types - the resolver takes no parameters.

Nov 29 2021, 6:56 AM · Restricted Project
erichkeane added a comment to D51650: Implement target_clones multiversioning.

Relanded: fc53eb69c26cdd7efa6b629c187d04326f0448ca

Nov 29 2021, 6:47 AM · Restricted Project
erichkeane added a reverting change for rGbb4934601d73: Revert "Implement target_clones multiversioning": rGfc53eb69c26c: Reapply 'Implement target_clones multiversioning'.
Nov 29 2021, 6:47 AM
erichkeane committed rGfc53eb69c26c: Reapply 'Implement target_clones multiversioning' (authored by erichkeane).
Reapply 'Implement target_clones multiversioning'
Nov 29 2021, 6:47 AM
erichkeane committed rG90010c2e1d60: Don't consider 'LinkageSpec' when calculating DeclContext 'Encloses' (authored by erichkeane).
Don't consider 'LinkageSpec' when calculating DeclContext 'Encloses'
Nov 29 2021, 6:25 AM
erichkeane closed D113709: Don't consider `LinkageSpec` when calculating DeclContext `Encloses`.
Nov 29 2021, 6:24 AM · Restricted Project

Nov 16 2021

erichkeane added a comment to D51650: Implement target_clones multiversioning.

Since it is not clear whether the semantic change was intended, I think it makes sense to revert the patch for now. If it is intended, it might be good to mention it in the change description, so that people are warned.

That looks like an unintended change to me, likely due to the new mutual exclusion checks. Thanks for letting us know!

Nov 16 2021, 9:29 PM · Restricted Project

Nov 11 2021

erichkeane requested review of D113709: Don't consider `LinkageSpec` when calculating DeclContext `Encloses`.
Nov 11 2021, 1:11 PM · Restricted Project
erichkeane committed rG9deab60ae710: Implement target_clones multiversioning (authored by erichkeane).
Implement target_clones multiversioning
Nov 11 2021, 11:11 AM
erichkeane closed D51650: Implement target_clones multiversioning.
Nov 11 2021, 11:11 AM · Restricted Project
erichkeane updated the diff for D51650: Implement target_clones multiversioning.

added C++ tests this time, changed how dupes are diagnosed.

Nov 11 2021, 10:15 AM · Restricted Project
erichkeane updated the diff for D51650: Implement target_clones multiversioning.

Did all the things Aaron asked for, but required adding 'lambda not supported yet' logic for this.

Nov 11 2021, 9:23 AM · Restricted Project
erichkeane accepted D113647: [X86] Honor command line features along with cpu_specific attribute.
Nov 11 2021, 8:28 AM · Restricted Project
erichkeane accepted D113647: [X86] Honor command line features along with cpu_specific attribute.

Is there a lit-test that could be added to make sure this happens? We put this in an llvm-attribute, so it should be checkable.

Nov 11 2021, 6:05 AM · Restricted Project

Nov 10 2021

erichkeane updated the diff for D51650: Implement target_clones multiversioning.

For the rest of multiversioning we count on the optimizer to remove variants made irrelevant, but I'm not sure opt can do anything like that yet :) But nit made.

Nov 10 2021, 9:30 AM · Restricted Project

Nov 9 2021

erichkeane accepted D113431: [clang][test][NFC] Move attr-ifunc.c test from Sema to CodeGen.

If you meant fusing the clang-format commit with this one, doing it in the same commit results in git no longer detecting the connection between them (similarity too low), so it loses the history. When searching I found a recommendation to avoid combining renames with modifications where git history matters, so I thought I might as well :)

Nov 9 2021, 1:38 PM · Restricted Project
erichkeane added a comment to D113431: [clang][test][NFC] Move attr-ifunc.c test from Sema to CodeGen.

I've got no problem with this, but it could just as easily have happened in the other review. Is there a reason not to?

Nov 9 2021, 12:11 PM · Restricted Project
erichkeane accepted D113504: [clang][test][NFC] clang-format attr-ifunc.c test.

This is something you can do as review-after-commit if you wish in the future.

Nov 9 2021, 12:08 PM · Restricted Project
erichkeane added a comment to D98895: [X86][clang] Disable long double type for -mno-x87 option.

This patch causes a regression.

To reproduce - clang -cc1 -fsycl-is-device -triple spir64 test.cpp

test.cpp:x:3: error: 'bar<__float128>' requires 128 bit size '__float128' type support, but target 'spir64' does not support it
T bar() { return T(); };
  ^

I looked at it briefly, and I believe the issue is call to checkTypeSupport() in ActOnFinishFunctionBody(). I tried deleting the call but it breaks tests (E.g. L26 in x86_64-no-x87.cpp). @asavonic Please take a look. I will be reverting the patch if this cannot be fixed soon.

The diagnostic seems to be correct - this instance of bar returns an unsupported type. Why do you think it should not be diagnosed?

I believe the problem is that there are now _3_ different diagnostics for the same thing, the one on 'bar', plus 2 more here:

auto malAutoTemp5 = bar<__float128>();

I think i would expect 1 error on 'bar', 1 error on the deduced 'auto', but the 3rd is superfluous.

I will check if there is a way to filter it out. However, I don't think that it is a good reason to revert the patch.

Nov 9 2021, 10:26 AM · Restricted Project
erichkeane added a comment to D98895: [X86][clang] Disable long double type for -mno-x87 option.

This patch causes a regression.

To reproduce - clang -cc1 -fsycl-is-device -triple spir64 test.cpp

test.cpp:x:3: error: 'bar<__float128>' requires 128 bit size '__float128' type support, but target 'spir64' does not support it
T bar() { return T(); };
  ^

I looked at it briefly, and I believe the issue is call to checkTypeSupport() in ActOnFinishFunctionBody(). I tried deleting the call but it breaks tests (E.g. L26 in x86_64-no-x87.cpp). @asavonic Please take a look. I will be reverting the patch if this cannot be fixed soon.

The diagnostic seems to be correct - this instance of bar returns an unsupported type. Why do you think it should not be diagnosed?

Nov 9 2021, 10:16 AM · Restricted Project

Nov 8 2021

erichkeane updated subscribers of D112349: [Verifier] Add verification logic for GlobalIFuncs.
Nov 8 2021, 2:43 PM · Restricted Project, Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

Who is the Clang CFE maintainer that we need to involve?

Nov 8 2021, 2:42 PM · Restricted Project, Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

To me the 'not in the weeds' part is, "How do I get CPU-dispatch/CPU-specific to work without RAUW, since that is offensive to the CFE code owner? Additionally, I found that some of the examples without the defined resolver actually DO work in my downstream, though I don't know what changes we make to make that happen. So adding this limitation in actually breaks my downstream.

Regarding the examples that do work, I've provided explanations as to how they partially sort-of-work, but that the general semantics are broken (the 'connection' to the resolver is 'severed' in each translation unit that has it as a declaration + all references from within that TU are borked).

Nov 8 2021, 12:56 PM · Restricted Project, Restricted Project
erichkeane added a comment to D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default.

Either this or D108453 (which were committed together!) caused this assert according to my git-bisect: https://godbolt.org/z/4rqYKfW7K

Nov 8 2021, 8:55 AM · Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

And how is Cling expecting CFE to deal with partial knowledge situations at the implementation level? To deal with exactly the non-local cases that the current violations address?
If there's no prescriptive way forward to dealing with these cases (so they're tech debt without a remediation plan), then as far as I'm concerned this case sits exactly under the same tech-debt umbrella of the existing violations and the way forward is using the same violating solution for this case too.
We definitely shouldn't block the IR verification indefinitely on this impasse. Once an acceptable solution is found, this will also be part of that refactor.

My understanding is that the REPL setup is that the 'IR' just needs to be in a state where it doesn't require reverts/rewrites later, so that we can do partial-back-end-code-generation as we go along. That said, I'm not positive as to the implications. I'm just repeating the discussion the CFE code-owner made at the time.

IMO, the 'acceptable' solution is to have a way to forward-declare an ifunc in IR so that we can fill in the resolver later on. From your description earlier, it seems that this would work as we could emit it as an 'unknown symbol' (as if it was an undefined function declaration), and would be completely implementable in the CFE.

So it would change your plan from earlier to:

When processing cpu_specific, emit the ifunc "x.ifunc", with no resolver;
When processing cpu_dispatch:

Get/Create the ifunc, then pull up the resolver.
If the resolver is null (as it should be), create one and update the 'ifunc'.
Generate said resolver.

Speaking of the incremental compilation case, we can experiment with the clang-repl binary. I am not sure about the details of this discussion but here is an example that works today:

llvm/build/bin/clang-repl 
clang-repl> __attribute__((cpu_specific(ivybridge))) void single_version(void){}
clang-repl> void useage() {single_version();}
clang-repl> quit

What would it be a good example to check if the incremental compilation case is covered?

Nov 8 2021, 5:55 AM · Restricted Project, Restricted Project
erichkeane accepted D112868: [CodeGen] Diagnose and reject non-function ifunc resolvers.
Nov 8 2021, 5:51 AM · Restricted Project

Nov 5 2021

erichkeane added a comment to D51650: Implement target_clones multiversioning.

Sadly, I think _I_ am the multiversioning expert (or at least, past-me was), so I'm hoping some of the reviewers @danielkiss can get to join will be able to read/understand this stuff for a quality review.

Nov 5 2021, 9:00 AM · Restricted Project
erichkeane updated the diff for D51650: Implement target_clones multiversioning.

Another rebase, as requested. I am not particularly familiar with this code anymore, so my responses to reviews might not be particularly well informed, but I'm hoping that rust shakes off through the review :)

Nov 5 2021, 8:45 AM · Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

And how is Cling expecting CFE to deal with partial knowledge situations at the implementation level? To deal with exactly the non-local cases that the current violations address?
If there's no prescriptive way forward to dealing with these cases (so they're tech debt without a remediation plan), then as far as I'm concerned this case sits exactly under the same tech-debt umbrella of the existing violations and the way forward is using the same violating solution for this case too.
We definitely shouldn't block the IR verification indefinitely on this impasse. Once an acceptable solution is found, this will also be part of that refactor.

Nov 5 2021, 8:30 AM · Restricted Project, Restricted Project
erichkeane added a comment to D112868: [CodeGen] Diagnose and reject non-function ifunc resolvers.

Explanations make sense to me, I'm generally in favor with the 1 concern.

Nov 5 2021, 6:01 AM · Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

I see. What is the guiding principle there, though? Generating correct IR "up front" / "the first time" rather than "fixing it up as you go via manipulations"? (could you give a link?)
I can see the engineering consideration in not letting IR manipulations creep into the CFE, I just want to make sure that's the principle that we're asked to follow.
In the end this isn't the first instance where the "streaming" design of GlobalDecl emission forces a fixup/rewrite of a previous decision, as can be evidenced by a close sibling of this feature, CodeGenModule::EmitAliasDefinition (which uses exactly that idiom, for a very similar use-case)...
It will always be either a fixup or an accumulate/commit idiom, since there will always be a GlobalDecl ordering where the information to make 'the perfect module-global decision' isn't available upon having to act on partial information.

In the end, I would like to have the verification of IFuncs having a defined resolver restored (and avoid more dependencies being taken on this being 'allowed' at the IR level), since having LLVM emit an object file with an undefined STT_GNU_IFUNC is probably just trouble and confusion waiting to happen.

Nov 5 2021, 5:58 AM · Restricted Project, Restricted Project

Nov 4 2021

erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

If the result wasn't null (call it F), use GI->takeName(F); F->replaceAllUsesWith(GI);

Nov 4 2021, 11:19 AM · Restricted Project, Restricted Project
erichkeane added a comment to D112349: [Verifier] Add verification logic for GlobalIFuncs.

I don't know much about the ELF format... but this works today? We can define a resolver in a different TU and it WORKS thanks to the linker? So there is perhaps something?

The ifunc symbol that is emitted in the TU with the undefined resolver loses its connection to the resolver and the calls to the ifunc are instead bound against the resolver itself (which is absolutely not what you want).

itay> cat specific.c
#include <stdio.h>

__attribute__((cpu_specific(generic)))
void single_version(void){
  puts("In single_version generic");
}

void useage() {
  single_version();
}

itay> cat dispatch_main.c
void useage(void);

__attribute__((cpu_dispatch(generic)))
void single_version(void);

int main()
{
  useage();
  single_version();
  return 0;
}

itay> clang -c dispatch_main.c -o dispatch_main.c.o
itay> clang -c specific.c -o specific.c.o
itay> clang specific.c.o dispatch_main.c.o -o main
itay> ./main
In single_version generic

This line should have been printed twice, not once.

Nov 4 2021, 9:30 AM · Restricted Project, Restricted Project