- User Since
- Jun 28 2016, 8:37 AM (290 w, 1 d)
No comments other than the ones others have made, though the make_signed/make_unsigned needs to work for _BitInt
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.
Fri, Jan 14
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.
Thu, Jan 13
CFE changes LGTM, the limits.h/.cpp changes LOOK right, but please give others a chance to take a look.
Wed, Jan 12
Mon, Jan 10
LGTM other than the NIT found by @fhahn
Fri, Jan 7
I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb
Thu, Jan 6
Tue, Jan 4
Should be fixed here: 2edc21e8566be8fa9b20e0bb71a83af90ec9aa97
Sorry for the delay, I just got back from my christmas break. I'll take a look later today/this week.
Dec 17 2021
Dec 16 2021
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 15 2021
Fix Aaron+ Craig's comments.
Implement the other two easily doable operators, do Craigs suggestions.
@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.
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 14 2021
Enable __int128 vectors, and fix the issue of the >64 bit vectors for negation. Also add a test for these.
Dec 13 2021
I DID just try to fix that thing:
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 10 2021
Dec 9 2021
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 8 2021
@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 7 2021
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.
A few nits, but not nearly 'expert' enough to approve without giving everyone else some time to look this over.
Can you add some tests please? Additionally, we don't typically use LLVM_UNLIKELY like you did there.
Dec 6 2021
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 2 2021
Dec 1 2021
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.
I just found that build failure in our downstream, and did a little investigation. Running the clang-invocations directly resulted in :
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.
Nov 30 2021
See: https://lists.llvm.org/pipermail/llvm-dev/2021-November/153882.html for what I was thinking of.
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 29 2021
Please ignore the clang-format suggestion.
I note that this is missing a test, otherwise I don't see any issues with it from my end.
Nov 16 2021
Nov 11 2021
added C++ tests this time, changed how dupes are diagnosed.
Did all the things Aaron asked for, but required adding 'lambda not supported yet' logic for this.
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 10 2021
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 9 2021
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?
This is something you can do as review-after-commit if you wish in the future.
Nov 8 2021
Nov 5 2021
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.
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 :)
Explanations make sense to me, I'm generally in favor with the 1 concern.
Nov 4 2021
If the result wasn't null (call it F), use GI->takeName(F); F->replaceAllUsesWith(GI);