[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory
lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. I didn't review the patch in detail, but +1 this is a great step forward to reorganize the repo! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131919/new/ https://reviews.llvm.org/D131919 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes
lattner added a comment. This is awesome, I agree completely we should curate release notes better. That said, I think this should make it more clear that there is a "difference in kind" between user-facing tools like clang/lldb etc and other libraries in LLVM. We don't want release note burden (or noise) for every little thing going into the optimizer or codegen. Do you think it would make sense to point out that this is about user-facing tools? Comment at: llvm/docs/DeveloperPolicy.rst:195 +* Adding, removing, or regrouping a diagnostic. +* Fixing a bug (please link to the issue fixed in the bug database). +* Adding or removing an optimization. aaron.ballman wrote: > nikic wrote: > > I disagree with this point. Bug fixes should not make it into the release > > notes as a matter of course -- there may be specific high-impact bug fixes > > that may be worth mentioning, but bug fixes should not be included in the > > release notes as a matter of policy. > > > > I agree that release notes for Clang/LLVM are currently insufficient, but > > we should also be careful not to over-compensate in the other direction, > > but including too much irrelevant noise. > > I disagree with this point. Bug fixes should not make it into the release > > notes as a matter of course -- there may be specific high-impact bug fixes > > that may be worth mentioning, but bug fixes should not be included in the > > release notes as a matter of policy. > > I disagree (quite strongly) with this and would point out that users have > explicitly requested this information be included: > https://discourse.llvm.org/t/rfc-update-developer-policy-on-release-notes/61856/3 > > > I agree that release notes for Clang/LLVM are currently insufficient, but > > we should also be careful not to over-compensate in the other direction, > > but including too much irrelevant noise. > > Bug fixes are generally far from irrelevant, but I agree that reviewer and > author discretion is advised (as with any release note). For example, if you > introduce a bug in version N and fix the bug in version N, there's no need > for a release note in that situation because there's no user-facing change as > far as users care about. But I don't want to try to spell that out in precise > detail because there will always be edge cases. I agree with both of you - listing every bug report would be too noisy and pretty irrelevant for most users, but there are high impact ones that are important and valuable to list. How about wording this bullet something like: * Fixing high impact bugs, e.g. ones that get discussed on hackernews or comparable forums (please link to the issue fixed in the bug database). Comment at: llvm/docs/DeveloperPolicy.rst:196 +* Fixing a bug (please link to the issue fixed in the bug database). +* Adding or removing an optimization. +* Modifying a C stable API. This is also too broad IMO, we don't want every new instcombine listed. I'd recommend making this more user-centric, e.g. "Adding or removing optimizations that have widespread impact or enables new programming paradigms" CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes
lattner added a comment. Also, when this lands, we should post on the forum about it. Every change to the developer policy warrants broader visibility than just a phab discussion IMO. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D123957: Update the developer policy to mention release notes
lattner accepted this revision. lattner added a comment. Nice, LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner added a comment. This patch has a lot of noise, the important part is APInt.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner marked an inline comment as done.
lattner added inline comments.
Comment at: llvm/include/llvm/ADT/APInt.h:384
/// value for the APInt's bit width.
bool isMaxValue() const { return isAllOnesValue(); }
craig.topper wrote:
> isAllOnes()?
Yep, good catch, fixed thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109483/new/
https://reviews.llvm.org/D109483
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner added inline comments.
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243
"Don't know how to expand this subtraction!");
-Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
- DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
- VT));
+Tmp1 = DAG.getNode(
+ISD::XOR, dl, VT, Node->getOperand(1),
craig.topper wrote:
> This could use DAG.getNOT if you're willing to make that change.
I'd prefer to keep this one separate, it isn't related to APInt.
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12185
else
- OtherOp = DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-VT);
+ OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
return true;
craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Will do this in a followup
Comment at: llvm/lib/Target/Lanai/LanaiISelLowering.cpp:1403
else
- OtherOp =
- DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl, VT);
+ OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
return true;
craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Likewise
Comment at: llvm/unittests/ADT/APIntTest.cpp:29
TEST(APIntTest, ShiftLeftByZero) {
- APInt One = APInt::getNullValue(65) + 1;
+ APInt One = APInt::getZero(65) + 1;
APInt Shl = One.shl(0);
craig.topper wrote:
> That's a strange way to write APInt(64, 1) unless there was some specific bug.
I agree, assume that this was testing some former bug. Unit tests are designed
to be weird ;-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109483/new/
https://reviews.llvm.org/D109483
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner added a comment. Thank you for the detailed review Craig! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner added a comment. I'll take care of the DAG.getAllOnesConstant change, but i'd appreciate it if you could look at the NOT cases. Running tests on the DAG.getAllOnesConstant patch now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.
lattner added a comment. > What is a "keep constructor"? Good question, I'm not sure. I think I meant to say "key constructors". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D110496: [llvm] [ADT] Add a range/iterator-based split()
lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. This is really nice! Please fix the clang-tidy casing issues, but otherwise LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110496/new/ https://reviews.llvm.org/D110496 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D110535: [llvm] [ADT] Update llvm::Split() per Pavel Labath's suggestions
lattner added a comment. Do you have commit access? If not, please read the llvm developer policy and follow the instructions, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110535/new/ https://reviews.llvm.org/D110535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity
lattner accepted this revision. lattner added a comment. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104819/new/ https://reviews.llvm.org/D104819 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D107528: [llvm][clang][NFC] updates inline licence info
lattner accepted this revision. lattner added a comment. Thank you for catching this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107528/new/ https://reviews.llvm.org/D107528 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
