[PATCH] D95299: Fix truncated __OPENMP_NVPTX__ preprocessor condition

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc49142e4f5c8: Fix truncated __OPENMP_NVPTX__ preprocessor 
condition (authored by r-burns, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95299/new/

https://reviews.llvm.org/D95299

Files:
  clang/lib/Headers/__clang_cuda_math.h


Index: clang/lib/Headers/__clang_cuda_math.h
===
--- clang/lib/Headers/__clang_cuda_math.h
+++ clang/lib/Headers/__clang_cuda_math.h
@@ -36,7 +36,7 @@
 // because the OpenMP overlay requires constexpr functions here but prior to
 // c++14 void return functions could not be constexpr.
 #pragma push_macro("__DEVICE_VOID__")
-#ifdef __OPENMP_NVPTX__ && defined(__cplusplus) && __cplusplus < 201402L
+#if defined(__OPENMP_NVPTX__) && defined(__cplusplus) && __cplusplus < 201402L
 #define __DEVICE_VOID__ static __attribute__((always_inline, nothrow))
 #else
 #define __DEVICE_VOID__ __DEVICE__


Index: clang/lib/Headers/__clang_cuda_math.h
===
--- clang/lib/Headers/__clang_cuda_math.h
+++ clang/lib/Headers/__clang_cuda_math.h
@@ -36,7 +36,7 @@
 // because the OpenMP overlay requires constexpr functions here but prior to
 // c++14 void return functions could not be constexpr.
 #pragma push_macro("__DEVICE_VOID__")
-#ifdef __OPENMP_NVPTX__ && defined(__cplusplus) && __cplusplus < 201402L
+#if defined(__OPENMP_NVPTX__) && defined(__cplusplus) && __cplusplus < 201402L
 #define __DEVICE_VOID__ static __attribute__((always_inline, nothrow))
 #else
 #define __DEVICE_VOID__ __DEVICE__
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95299: Fix truncated __OPENMP_NVPTX__ preprocessor condition

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.
Herald added a subscriber: wangpc.

@r-burns : what should I use for the authorship (name/pseudo and email)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95299/new/

https://reviews.llvm.org/D95299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155509: Revert "Remove rdar links; NFC"

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe0ac46e69d7a: Revert Remove rdar links; NFC 
(authored by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155509/new/

https://reviews.llvm.org/D155509

Files:
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
  clang/test/ARCMT/GC-check-warn-nsalloc.m
  clang/test/ARCMT/GC-no-finalize-removal.m
  clang/test/ARCMT/GC-no-finalize-removal.m.result
  clang/test/ARCMT/GC.m
  clang/test/ARCMT/GC.m.result
  clang/test/ARCMT/check-with-pch.m
  clang/test/ARCMT/checking.m
  clang/test/ARCMT/no-canceling-bridge-to-bridge-cast.m
  clang/test/ARCMT/nonobjc-to-objc-cast-2.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m.result
  clang/test/ARCMT/objcmt-atomic-property.m
  clang/test/ARCMT/objcmt-atomic-property.m.result
  clang/test/ARCMT/objcmt-boxing.m
  clang/test/ARCMT/objcmt-boxing.m.result
  clang/test/ARCMT/objcmt-migrate-all.m
  clang/test/ARCMT/objcmt-migrate-all.m.result
  clang/test/ARCMT/objcmt-ns-macros.m
  clang/test/ARCMT/objcmt-ns-macros.m.result
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m.result
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m.result
  clang/test/ARCMT/objcmt-property-availability.m
  clang/test/ARCMT/objcmt-property-availability.m.result
  clang/test/ARCMT/objcmt-property-dot-syntax.m
  clang/test/ARCMT/objcmt-property-dot-syntax.m.result
  clang/test/ARCMT/objcmt-property.m
  clang/test/ARCMT/objcmt-property.m.result
  clang/test/ARCMT/objcmt-protocol-conformance.m
  clang/test/ARCMT/objcmt-protocol-conformance.m.result
  clang/test/ARCMT/objcmt-undefined-ns-macros.m
  clang/test/ARCMT/objcmt-undefined-ns-macros.m.result
  clang/test/Analysis/DeallocMissingRelease.m
  clang/test/Analysis/DeallocUseAfterFreeErrors.m
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
  clang/test/Analysis/Inputs/expected-plists/malloc-plist.c.plist
  clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Analysis/NSString.m
  clang/test/Analysis/OSAtomic_mac.cpp
  clang/test/Analysis/PR46264.cpp
  clang/test/Analysis/UserNullabilityAnnotations.m
  clang/test/Analysis/array-struct-region.c
  clang/test/Analysis/blocks.m
  clang/test/Analysis/call-and-message.m
  clang/test/Analysis/call-invalidation.cpp
  clang/test/Analysis/cfref_rdar6080742.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/default-analyze.m
  clang/test/Analysis/delegates.m
  clang/test/Analysis/edges-new.mm
  clang/test/Analysis/generics.m
  clang/test/Analysis/inline-plist.c
  clang/test/Analysis/inlining/Inputs/expected-plists/path-notes.m.plist
  clang/test/Analysis/inlining/eager-reclamation-path-notes.c
  clang/test/Analysis/inlining/false-positive-suppression.c
  clang/test/Analysis/inlining/path-notes.m
  clang/test/Analysis/malloc-interprocedural.c
  clang/test/Analysis/malloc-plist.c
  clang/test/Analysis/malloc.c
  clang/test/Analysis/misc-ps-64.m
  clang/test/Analysis/misc-ps-arm.m
  clang/test/Analysis/misc-ps-eager-assume.m
  clang/test/Analysis/misc-ps-ranges.m
  clang/test/Analysis/misc-ps-region-store.cpp
  clang/test/Analysis/misc-ps-region-store.m
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/mutually_exclusive_null_fp.cpp
  clang/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret-region.m
  clang/test/Analysis/null-deref-ps.c
  clang/test/Analysis/objc-arc.m
  clang/test/Analysis/objc-encode.m
  clang/test/Analysis/objc-subscript.m
  clang/test/Analysis/osobject-retain-release.cpp
  clang/test/Analysis/plist-output-alternate.m
  clang/test/Analysis/plist-output.m
  clang/test/Analysis/properties.m
  clang/test/Analysis/properties.mm
  clang/test/Analysis/reference.cpp
  clang/test/Analysis/retain-release-inline.m
  clang/test/Analysis/retain-release-region-store.m
  clang/test/Analysis/retain-release.m
  clang/test/Analysis/retain-release.mm
  clang/test/Analysis/security-syntax-checks.m
  clang/test/Analysis/stack-addr-ps.c
  clang/test/Analysis/stack-addr-ps.cpp
  clang/test/Analysis/templates.cpp
  clang/test/Analysis/uninit-bug-first-iteration-init.c
  

[PATCH] D155509: Revert "Remove rdar links; NFC"

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini created this revision.
mehdi_amini added reviewers: lattner, aaron.ballman.
Herald added subscribers: steakhal, jdoerfert, martong, pengfei, arphaman.
Herald added a reviewer: NoQ.
Herald added a project: All.
mehdi_amini requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

This reverts commit d618f1c3b12effd0c2bdb7d02108d3551f389d3d 
.
This commit wasn't reviewed ahead of time and significant concerns were
raised immediately after it landed. According to our developer policy
this warrants immediate revert of the commit.

https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155509

Files:
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
  clang/test/ARCMT/GC-check-warn-nsalloc.m
  clang/test/ARCMT/GC-no-finalize-removal.m
  clang/test/ARCMT/GC-no-finalize-removal.m.result
  clang/test/ARCMT/GC.m
  clang/test/ARCMT/GC.m.result
  clang/test/ARCMT/check-with-pch.m
  clang/test/ARCMT/checking.m
  clang/test/ARCMT/no-canceling-bridge-to-bridge-cast.m
  clang/test/ARCMT/nonobjc-to-objc-cast-2.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m
  clang/test/ARCMT/objcmt-arc-cf-annotations.m.result
  clang/test/ARCMT/objcmt-atomic-property.m
  clang/test/ARCMT/objcmt-atomic-property.m.result
  clang/test/ARCMT/objcmt-boxing.m
  clang/test/ARCMT/objcmt-boxing.m.result
  clang/test/ARCMT/objcmt-migrate-all.m
  clang/test/ARCMT/objcmt-migrate-all.m.result
  clang/test/ARCMT/objcmt-ns-macros.m
  clang/test/ARCMT/objcmt-ns-macros.m.result
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m
  clang/test/ARCMT/objcmt-ns-nonatomic-iosonly.m.result
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m
  clang/test/ARCMT/objcmt-ns-returns-inner-pointer.m.result
  clang/test/ARCMT/objcmt-property-availability.m
  clang/test/ARCMT/objcmt-property-availability.m.result
  clang/test/ARCMT/objcmt-property-dot-syntax.m
  clang/test/ARCMT/objcmt-property-dot-syntax.m.result
  clang/test/ARCMT/objcmt-property.m
  clang/test/ARCMT/objcmt-property.m.result
  clang/test/ARCMT/objcmt-protocol-conformance.m
  clang/test/ARCMT/objcmt-protocol-conformance.m.result
  clang/test/ARCMT/objcmt-undefined-ns-macros.m
  clang/test/ARCMT/objcmt-undefined-ns-macros.m.result
  clang/test/Analysis/DeallocMissingRelease.m
  clang/test/Analysis/DeallocUseAfterFreeErrors.m
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
  clang/test/Analysis/Inputs/expected-plists/malloc-plist.c.plist
  clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objc.plist
  clang/test/Analysis/Inputs/expected-plists/retain-release.m.objcpp.plist
  clang/test/Analysis/NSString.m
  clang/test/Analysis/OSAtomic_mac.cpp
  clang/test/Analysis/PR46264.cpp
  clang/test/Analysis/UserNullabilityAnnotations.m
  clang/test/Analysis/array-struct-region.c
  clang/test/Analysis/blocks.m
  clang/test/Analysis/call-and-message.m
  clang/test/Analysis/call-invalidation.cpp
  clang/test/Analysis/cfref_rdar6080742.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/default-analyze.m
  clang/test/Analysis/delegates.m
  clang/test/Analysis/edges-new.mm
  clang/test/Analysis/generics.m
  clang/test/Analysis/inline-plist.c
  clang/test/Analysis/inlining/Inputs/expected-plists/path-notes.m.plist
  clang/test/Analysis/inlining/eager-reclamation-path-notes.c
  clang/test/Analysis/inlining/false-positive-suppression.c
  clang/test/Analysis/inlining/path-notes.m
  clang/test/Analysis/malloc-interprocedural.c
  clang/test/Analysis/malloc-plist.c
  clang/test/Analysis/malloc.c
  clang/test/Analysis/misc-ps-64.m
  clang/test/Analysis/misc-ps-arm.m
  clang/test/Analysis/misc-ps-eager-assume.m
  clang/test/Analysis/misc-ps-ranges.m
  clang/test/Analysis/misc-ps-region-store.cpp
  clang/test/Analysis/misc-ps-region-store.m
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/mutually_exclusive_null_fp.cpp
  clang/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret-region.m
  clang/test/Analysis/null-deref-ps.c
  clang/test/Analysis/objc-arc.m
  clang/test/Analysis/objc-encode.m
  clang/test/Analysis/objc-subscript.m
  clang/test/Analysis/osobject-retain-release.cpp
  clang/test/Analysis/plist-output-alternate.m
  clang/test/Analysis/plist-output.m
  clang/test/Analysis/properties.m
  

[PATCH] D155081: Specify the developer policy around links to external resources

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

Approving in support, but don't consider this enough as-is. I suspect you 
should get @lattner approval here :)




Comment at: llvm/docs/DeveloperPolicy.rst:357
 
-* If the patch has been reviewed, add a link to its review page, as shown
+* It is acceptable to add metadata to the commit message to automate processes.
+  If the patch fixes a bug in GitHub Issues, we encourage adding

More explicitly: "automate processes, including for downstream consumers"?



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

ldionne wrote:
> smeenai wrote:
> > aaron.ballman wrote:
> > > arsenm wrote:
> > > > I haven't quite figured out what the exact syntaxes which are 
> > > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > > Yup, it does support that form as well. I had heard more than once during 
> > > code review that folks seem to prefer the full link because it's easier 
> > > to click on that from the commit message than it is to navigate to the 
> > > fix from the number alone. That seemed like a pretty good reason to 
> > > recommend the full form, but I don't have strong opinions.
> > +1 for encouraging the full link
> Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> around with the Github transition or not?
@arsenm: It's documented 
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
And for linking cross-repo: 
https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155081/new/

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG55c2211a233e: [APFloat] Add APFloat semantic support for 
TF32 (authored by jfurtek, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151923/new/

https://reviews.llvm.org/D151923

Files:
  clang/lib/AST/MicrosoftMangle.cpp
  llvm/include/llvm/ADT/APFloat.h
  llvm/lib/Support/APFloat.cpp
  llvm/unittests/ADT/APFloatTest.cpp

Index: llvm/unittests/ADT/APFloatTest.cpp
===
--- llvm/unittests/ADT/APFloatTest.cpp
+++ llvm/unittests/ADT/APFloatTest.cpp
@@ -682,6 +682,26 @@
 EXPECT_TRUE(T.isDenormal());
 EXPECT_EQ(fcPosSubnormal, T.classify());
   }
+
+  // Test TF32
+  {
+const char *MinNormalStr = "1.17549435082228750797e-38";
+EXPECT_FALSE(APFloat(APFloat::FloatTF32(), MinNormalStr).isDenormal());
+EXPECT_FALSE(APFloat(APFloat::FloatTF32(), 0).isDenormal());
+
+APFloat Val2(APFloat::FloatTF32(), 2);
+APFloat T(APFloat::FloatTF32(), MinNormalStr);
+T.divide(Val2, rdmd);
+EXPECT_TRUE(T.isDenormal());
+EXPECT_EQ(fcPosSubnormal, T.classify());
+
+const char *NegMinNormalStr = "-1.17549435082228750797e-38";
+EXPECT_FALSE(APFloat(APFloat::FloatTF32(), NegMinNormalStr).isDenormal());
+APFloat NegT(APFloat::FloatTF32(), NegMinNormalStr);
+NegT.divide(Val2, rdmd);
+EXPECT_TRUE(NegT.isDenormal());
+EXPECT_EQ(fcNegSubnormal, NegT.classify());
+  }
 }
 
 TEST(APFloatTest, IsSmallestNormalized) {
@@ -1350,6 +1370,16 @@
 {   0x80ULL, APFloat::Float8E4M3B11FNUZ(), false, true, 0xaaULL },
 {   0x80ULL, APFloat::Float8E4M3B11FNUZ(), true, false, 0xaaULL },
 {   0x80ULL, APFloat::Float8E4M3B11FNUZ(), true, true,  0xaaULL },
+{0x3fe00ULL, APFloat::FloatTF32(), false, false,  0xULL },
+{0x7fe00ULL, APFloat::FloatTF32(), false,  true,  0xULL },
+{0x3feaaULL, APFloat::FloatTF32(), false, false,0xaaULL },
+{0x3ffaaULL, APFloat::FloatTF32(), false, false,   0xdaaULL },
+{0x3ffaaULL, APFloat::FloatTF32(), false, false,  0xfdaaULL },
+{0x3fd00ULL, APFloat::FloatTF32(),  true, false,  0xULL },
+{0x7fd00ULL, APFloat::FloatTF32(),  true,  true,  0xULL },
+{0x3fcaaULL, APFloat::FloatTF32(),  true, false,0xaaULL },
+{0x3fdaaULL, APFloat::FloatTF32(),  true, false,   0xfaaULL },
+{0x3fdaaULL, APFloat::FloatTF32(),  true, false,   0x1aaULL },
   // clang-format on
   };
 
@@ -1780,6 +1810,8 @@
 APFloat::getLargest(APFloat::Float8E5M2FNUZ()).convertToDouble());
   EXPECT_EQ(
   30, APFloat::getLargest(APFloat::Float8E4M3B11FNUZ()).convertToDouble());
+  EXPECT_EQ(3.40116213421e+38f,
+APFloat::getLargest(APFloat::FloatTF32()).convertToFloat());
 }
 
 TEST(APFloatTest, getSmallest) {
@@ -1831,6 +1863,13 @@
   EXPECT_TRUE(test.isFiniteNonZero());
   EXPECT_TRUE(test.isDenormal());
   EXPECT_TRUE(test.bitwiseIsEqual(expected));
+
+  test = APFloat::getSmallest(APFloat::FloatTF32(), true);
+  expected = APFloat(APFloat::FloatTF32(), "-0x0.004p-126");
+  EXPECT_TRUE(test.isNegative());
+  EXPECT_TRUE(test.isFiniteNonZero());
+  EXPECT_TRUE(test.isDenormal());
+  EXPECT_TRUE(test.bitwiseIsEqual(expected));
 }
 
 TEST(APFloatTest, getSmallestNormalized) {
@@ -1905,6 +1944,14 @@
   EXPECT_FALSE(test.isDenormal());
   EXPECT_TRUE(test.bitwiseIsEqual(expected));
   EXPECT_TRUE(test.isSmallestNormalized());
+
+  test = APFloat::getSmallestNormalized(APFloat::FloatTF32(), false);
+  expected = APFloat(APFloat::FloatTF32(), "0x1p-126");
+  EXPECT_FALSE(test.isNegative());
+  EXPECT_TRUE(test.isFiniteNonZero());
+  EXPECT_FALSE(test.isDenormal());
+  EXPECT_TRUE(test.bitwiseIsEqual(expected));
+  EXPECT_TRUE(test.isSmallestNormalized());
 }
 
 TEST(APFloatTest, getZero) {
@@ -1936,7 +1983,9 @@
   {::Float8E4M3FNUZ(), false, false, {0, 0}, 1},
   {::Float8E4M3FNUZ(), true, false, {0, 0}, 1},
   {::Float8E4M3B11FNUZ(), false, false, {0, 0}, 1},
-  {::Float8E4M3B11FNUZ(), true, false, {0, 0}, 1}};
+  {::Float8E4M3B11FNUZ(), true, false, {0, 0}, 1},
+  {::FloatTF32(), false, true, {0, 0}, 1},
+  {::FloatTF32(), true, true, {0x4ULL, 0}, 1}};
   const unsigned NumGetZeroTests = std::size(GetZeroTest);
   for (unsigned i = 0; i < NumGetZeroTests; ++i) {
 APFloat test = APFloat::getZero(*GetZeroTest[i].semantics,
@@ -6229,6 +6278,34 @@
   EXPECT_TRUE(std::isnan(QNaN.convertToDouble()));
 }
 
+TEST(APFloatTest, FloatTF32ToDouble) {
+  APFloat One(APFloat::FloatTF32(), "1.0");
+  EXPECT_EQ(1.0, One.convertToDouble());
+  APFloat PosLargest = 

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

Ok, sorry I missed the mangle update, seeing how I missed the MLIR mention as 
well: I need more sleep!! :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151923/new/

https://reviews.llvm.org/D151923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> Also do you have plans for MLIR? (nothing required for this patch, just 
> curious).

Actually it's the last sentence of the patch description (/facepalm)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151923/new/

https://reviews.llvm.org/D151923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151923: [APFloat] Add APFloat semantic support for TF32

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

I think this LG, but David had to update clang/lib/AST/MicrosoftMangle.cpp when 
some other float format was added, you don't need this here?

Also do you have plans for MLIR? (nothing required for this patch, just 
curious).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151923/new/

https://reviews.llvm.org/D151923

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1491
+mapTypes = exitDataOp.getMapTypes();
+mapperFunc = false;
+return success();

This line is not needed after the fix you pushed right?

Seems like we could also just set `bool mapperFunc = 
isa(op);` or something like that.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

TIFitis wrote:
> TIFitis wrote:
> > mehdi_amini wrote:
> > > mapperFunc is used uninitialized here which is UB, can you look into this?
> > Thanks for pointing out, I'll push a patch to fix this.
> https://github.com/llvm/llvm-project/commit/9ea3fcfa380c6097fddd0d9a9b2c13f0f20bc41a
> 
> This fixes it.
Thanks for the quick fix!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142914/new/

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.
Herald added a subscriber: bviyer.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1533
+ompLoc, builder.saveIP(), mapTypeFlags, mapNames, mapperAllocas,
+mapperFunc, deviceID, ifCond, processMapOpCB, bodyCB));
+  } else {

mapperFunc is used uninitialized here which is UB, can you look into this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142914/new/

https://reviews.llvm.org/D142914

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146041: initial commit

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.
Herald added a subscriber: JDevlieghere.

You should look into the title and description of the commit: 
https://cbea.ms/git-commit/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146041/new/

https://reviews.llvm.org/D146041

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141098: [clang-format][NFC] Set LineEnding to LF in config files

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D141098#4111809 , @owenpan wrote:

> Can we just fix the buildbot so that it runs git-clang-format with 
> `--binary`? Using clang-format-15 to format a patch to clang-format-16 source 
> still looks wrong to me.

The general principle is that developers of LLVM 16 should be able to use 
clang-format from the previous release (that is: one where we have prebuilt 
binaries available) to format patches in general.
It does not seem reasonable to me that we should need to build clang-format 
ourselves for the purpose of writing patches to LLVM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141098/new/

https://reviews.llvm.org/D141098

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141298: Move from llvm::makeArrayRef to ArrayRef deduction guides - last part

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Seems mechanical, and if it build everywhere LGTM :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141298/new/

https://reviews.llvm.org/D141298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.
Herald added a subscriber: Michael137.



Comment at: llvm/include/llvm/ADT/ArrayRef.h:502
+  /// Deduction guide to construct an ArrayRef from a C array.
+  template  ArrayRef(const T ()[N]) -> ArrayRef;
 

Can we keep the makeArrayRef functions for now and mark them deprecated?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140896/new/

https://reviews.llvm.org/D140896

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134878: Update developer policy on potentially breaking changes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

From what I saw when you posted the discourse thread initially, I understand 
you're targeting user-visible features, and mostly from the "toolchain" side of 
the project.
However I find the language here to be potentially confusing for the API 
surface of the libraries: that is how much of this applies to the LLVM C++ 
libraries API surface?
If this is out-of-scope, can this be called out more explicitly?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134878/new/

https://reviews.llvm.org/D134878

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131172: [clang][llvm][doc] Add more information for the ABI change in FP16

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:206
+When you find failures with ``half`` type, check the calling conversion of the
+code and switch it to the new ABI.
+

(same here: users reading "switch it to the new ABI" will be confused as to 
what to do exactly)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131172/new/

https://reviews.llvm.org/D131172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131172: [clang][llvm][doc] Add more information for the ABI change in FP16

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:633
+- If you are using downstream runtimes that provide FP16 conversions, update
+them with the new ABI.
+

Can you add a link to a documentation that describe the new ABI?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131172/new/

https://reviews.llvm.org/D131172

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> There should be a better way than this. Comprehensive pre-merge testing of 
> all PRs etc.

We already have pre-commit tests on Phabricator on Windows and Linux, but 
that's not exhaustive and for many reasons I don't believe this is realistic in 
any way: we will always have some post-commit buildbots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> Agreed, I think we need to update the protocol for changing the C++ standard 
> in the future to account for more testing beforehand.

The way I would do it is: wait for a Sunday so that the bots aren't loaded, 
land the change, wait for a couple of hours to ensure that all bots have picked 
up the revision, then revert and survey the results for the runs. Communicating 
ahead of time on this also so that downstream can pickup the revision for their 
own tests if they want.
This should provide a pretty good signal ahead of time of the "real" migration 
hopefully!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> . I haven't thought too hard about the performance of that while loop but it 
> seems good enough to land for now.

What's the finality of it? That is: outside of canonicalization what is its 
purpose?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D130689#3686760 , @h-vetinari 
wrote:

> My point boils down to: "written using standard C++17
> code" does not sound at all like "core language, no stdlib", but very much 
> like "core+stdlib".

We're allowing C++17 library feature, this isn't covered by the "vendor 
extensions" part but by the following paragraph:

> Nevertheless, we restrict ourselves to features which are available in the 
> major toolchains supported as host compilers

This includes not only missing features in libstdc++ but also gcc and clang 
bugs/limitations that we'll have to work around.

> This is also the first time this split becomes relevant AFAIK

I don't : the migration to C++11 was done the same way, piecewise by making 
sure that when we start using a new feature (core or stdlib) it actually works 
on the stated minimum version of the toolchains we support.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D130689#3684333 , @thieta wrote:

> In D130689#3684330 , @mehdi_amini 
> wrote:
>
>> What does it mean exactly? We can't use **anything** C++17 without writing 
>> it in the coding standards?
>> I'm not sure it'll be manageable: how do you see this playing out?
>
> Probably poorly worded - what I was trying to convey is that if we want to 
> use a C++17 feature that's really impactful on the syntax/readability we 
> should probably consider recommending ways to use it in the coding standards, 
> similar to our guidelines on using for() loops 
> (https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible)
>  or the auto keyword 
> (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable).

Makes sense, thanks! Let's just update the wording to convey this and this all 
looks good to me :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.
Herald added a subscriber: JDevlieghere.

Nice, LGTM, thanks for driving this!

> Remember that if we want to adopt some new feature in a bigger way it should 
> be discussed and added to the CodingStandard document.

What does it mean exactly? We can't use **anything** C++17 without writing it 
in the coding standards?
I'm not sure it'll be manageable: how do you see this playing out?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

For this to be a usable canonicalization, it is really the case where the 
operands are already sorted (no-op) that needs to be heavily optimized (that is 
no complex data structure to populate, etc.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Right now I'm not yet understanding all of the algorithm (haven't spent enough 
time on it), but I'm mostly concerned by the runtime cost of this normalization.




Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:371
+// assigned a sorted position yet.
+SmallVector bfsOfOperands;
+





Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:377-379
+  OperandBFS *bfsOfOperand = new OperandBFS();
+  bfsOfOperand->pushAncestor(operand.getDefiningOp());
+  bfsOfOperands.push_back(bfsOfOperand);




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:211
+template 
+class SortCommutativeOperands : public OpRewritePattern {
+  using OpRewritePattern::OpRewritePattern;

Can we make this not a template? This will be a code bloat otherwise.



Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:332
+
+} // namespace mlir
+

There seems to me to be far too much code in the public head: I can't isolate 
the public API here, if this is just about a pattern then the usual way is to 
have a "populate" method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107082: [X86][RFC] Enable `_Float16` type support on X86 following the psABI

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

This broke the bot here: 
https://lab.llvm.org/buildbot/#/builders/61/builds/27616

The cmake invocation includes some GPU specific options that you can omit 
(`-DMLIR_ENABLE_CUDA_RUNNER=1` , 
`-DCMAKE_CUDA_COMPILER=/usr/local/cuda/bin/nvcc`, 
`-DMLIR_ENABLE_VULKAN_RUNNER=1`, `-DMLIR_RUN_CUDA_TENSOR_CORE_TESTS=ON`), which 
should leave out:

  cmake ../llvm.src/llvm -DLLVM_BUILD_EXAMPLES=ON 
'-DLLVM_TARGETS_TO_BUILD=host;NVPTX' -DLLVM_ENABLE_PROJECTS=mlir  
-DMLIR_INCLUDE_INTEGRATION_TESTS=ON  -DBUILD_SHARED_LIBS=ON 
-DLLVM_CCACHE_BUILD=ON -DMLIR_ENABLE_BINDINGS_PYTHON=ON  
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_LIT_ARGS=-v -vv' 
-GNinja

*y


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107082/new/

https://reviews.llvm.org/D107082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126158: [MLIR][GPU] Replace fdiv on fp16 with promoted (fp32) multiplication with reciprocal plus one (conditional) Newton iteration.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

The shared library build was broken, I had to revert: 
https://lab.llvm.org/buildbot/#/builders/61/builds/27377


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126158/new/

https://reviews.llvm.org/D126158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Forgot to mention, we configure this build with:

  cmake -G Ninja ${src_dir}/llvm -DLLVM_TARGETS_TO_BUILD="X86;NVPTX;AMDGPU" 
-DLLVM_ENABLE_PROJECTS="lld;clang;mlir" -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_ASSERTIONS=Off -DLIBCXX_CXX_ABI=default 
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi"  -DLLVM_ENABLE_LLD=ON  
-DLLVM_DISTRIBUTION_COMPONENTS="clang;runtimes" -DLLVM_CCACHE_BUILD=ON

I guess `-DLIBCXX_CXX_ABI=default` is the issue, I'm not sure what we should 
use here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

I hit a config failure after f8c8bda965dd0f622de1ad3863b728661af6eb72 


  CMake Error at 
/var/lib/buildkite-agent/builds/buildkite-588bd64db9-mk2vq-1/mlir/mlir-core/libcxx/CMakeLists.txt:225
 (message):
Unsupported C++ ABI library: 'default'.  Supported values are
none;libcxxabi;system-libcxxabi;libcxxrt;libstdc++;libsupc++;vcruntime.
   

Here: 
https://buildkite.com/mlir/mlir-core/builds/22800#018100ac-56d6-4051-95ea-7dc91b98e658/14

We build from a clean directory, there is no CMake cache involved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Thanks for improving the doc! Are you moving this to be used in 
canonicalization next?
I think a first good step would be to make it a pattern and test it with a pass 
that applies it in the greedy rewriter. I would land this first and then try to 
enable this in the canonicalized.

Also, have you thought already about how to get rid of string manipulation?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

That was the sense of my question about canonicalization indeed :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D124750#3502401 , @srishti-pm 
wrote:

> In D124750#3502295 , @mehdi_amini 
> wrote:
>
>> You're telling me "what" while I'm actually more interested in the "why" 
>> here?
>
> I'm not sure what your question is, with a "why". Let me think about this a 
> bit. I'll get back to you.

My "why?" question is about canonicalization: could this be a canonicalization 
and if so why / why not? This is an important thing to answer before looking 
into the discussion below actually:

>> Same as before: this does not tell me why, can you provide an example where 
>> this matters?
>
> Sure. This is a bit lengthy. I'm really sorry for that !

No worry, thanks for being thorough.

> ...
> So, in essence, we can see that the effort of an end user writing a C++ 
> pattern has reduced if I sort the producers as well.

So far you explained "what is canonicalization and why are we canonicalizing", 
the same rationale applies to "push constant to the right" that we do already 
in canonicalization, and this is exactly why I asked before whether we could do 
what you're doing as a canonicalization.

> The real reason for sorting the producers is that, if such a thing is not 
> done, the sorting and this entire utility will be virtually useless.

So: I understand that the producers should be sorted for a pattern to apply, 
**but** our disconnect earlier is that my usual approach to see 
canonicalization is to process an entire block/region, and as such we don't 
work on slices but on operation in order until fix-point. I'm a bit concerned 
about efficiency of your approach, because when integrated in a framework that 
actually visit the entire block you would recompute subset of the same slice 
over and over and re-attempt to sort everything multiple times:

> d = add b, c
> s = add d, f
> g = add s, d
> h = add d, x
> i = add g, h

Every time the algorithm would consider a commutative op, that is all the op, 
it would recurse and try to re-sort the current slice, processing the same ops 
over and over.

> A deterministic sorting of an op requires its producers to be sorted.

This isn't clear to me why based on the sorting criteria you provided, but in 
any case a local sort with fixed-point iteration on an isolated region should 
converge (hopefully).

> Our sorting algorithm is based on the breadth-first traversal of backard 
> slices. On the same level of DAG, the traversal looks at operands from the 
> first to the last. That is how the breadth-first traversal is defined here. 
> Now, if this traversal is non-deterministic, then, the whole use of sorting 
> something will collapse. **Maybe, this can be BEST explained with the below 
> example.**
>
> If I have this IR:
> d = div b, c
> s = sub e, f
> x = xor k, l
> g = add s, d
> h = add d, x
> i = add g, h
>
> Then, `i = add g, h` will be sorted to `i = add g, h` (no change).
>
> **But**, when I have the below IR (which is functionally the same as the 
> above IR):
> d = div b, c
> s = sub e, f
> x = xor k, l
> g = add d, s
> h = add d, x
> i = add g, h
>
> Then, `i = add g, h` will be sorted to `i = add h, g`.

Why? Again your description of the sort is as follow:

> (1) the operands defined by non-constant-like ops come first, followed by
>  (2) block arguments, and these are followed by
> (3) the operands defined by constant-like ops. 
> In addition to this, within the category (1), the order of operands is 
> alphabetical w.r.t. the dialect name and op name.

In your example:

  g = add d, s
  h = add d, x
  i = add g, h

The operands fits category 1) I believe, and so they should be sorted 
"alphabetical w.r.t. the dialect name and op name", so a stable sort would 
never re-order them in any way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D124750#3502228 , @srishti-pm 
wrote:

> In D124750#3500748 , @mehdi_amini 
> wrote:
>
>> Seems like this should be added to canonicalization? The "push constants to 
>> the right hand side" is there already.
>
> I think this was not added to canonicalization because we wanted it to be an 
> independent utility that can be used if needed and not be used if not needed.

You're telling me "what" while I'm actually more interested in the "why" here?

>> I also don't understand the complexity of the implementation, I may need an 
>> example to understand why you're recursively operating on the producer ops 
>> for the operands.
>> From the revision description: (1) the operands defined by non-constant-like 
>> ops come first, followed by (2) block arguments, and these are followed by 
>> (3) the operands defined by constant-like ops` which all seems like a fairly 
>> local check: a stable-sort on the operands would be deterministic and local 
>> to a single operation.
>
> I do this because, firstly, in the description, if you look below this 
> paragraph, you will see the following:
> "And, if two operands come from the same op, the function backtracks and
> looks even further to sort them. This backtracking is done over the
> backward slice of the operand, in a breadth-first traversal."

Same as before: this does not tell me why, can you provide an example where 
this matters?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Seems like this should be added to canonicalization? The "push constants to the 
right hand side" is there already.

I also don't understand the complexity of the implementation, I may need an 
example to understand why you're recursively operating on the producer ops for 
the operands.
From the revision description: `, (1) the operands defined by non-constant-like 
ops come first, followed by (2) block arguments, and these are followed by (3) 
the operands defined by constant-like ops` which all seems like a fairly local 
check: a stable-sort on the operands would be deterministic and local to a 
single operation.




Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:235
+  bfsOfOperand->key = (Twine(bfsOfOperand->key) + Twine("1") +
+   
std::string(frontAncestor->getName().getStringRef()))
+  .str();

The string conversion seems unnecessary to me?



Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:271
+  for (Operation *operandDefOp : operandDefOps) {
+OperandBFS *bfsOfOperand = new OperandBFS();
+bfsOfOperand->pushAncestor(operandDefOp);

Is this a leak?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124750/new/

https://reviews.llvm.org/D124750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D119136#3449325 , @cor3ntin wrote:

> Yes, working on a fix as we speak.
> The meaning of that code changed (in all c++ language modes), I'm
> currently trying to find if we have any other issue of that sort and will
> commit a fix in a few hours

OK, I'll revert in the meantime to unbreak bots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Seems like this is breaking clang bootstrapping?

  llvm/include/llvm/CodeGen/LiveInterval.h:630:53: error: captured variable 
'Idx' cannot appear here
[=](std::remove_reference_t V,
  ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123297: [flang][driver] Add support for -mmlir

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

`-mmlir` is fine with me, but note that MLIR has much less global options than 
LLVM: you will only get access to context and passmanager options and not 
individual passes flags. That's not a criticism :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123297/new/

https://reviews.llvm.org/D123297

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121549: Define ABI breaking class members correctly

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121549/new/

https://reviews.llvm.org/D121549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121549: Define ABI breaking class members correctly

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.h:601
 
+  // TODO: place logger under #if LLVM_ENABLE_ABI_BREAKING_CHECKS
 #ifndef NDEBUG

This is a private header (you're in the `lib` directory and not in the 
`include` directory)

Same for the SystemZ one above I believe.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121549/new/

https://reviews.llvm.org/D121549

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG036088fd6ea2: [MLIR][Python] Add SCFIfOp Python binding 
(authored by chhzh123, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121076/new/

https://reviews.llvm.org/D121076

Files:
  mlir/python/mlir/dialects/_scf_ops_ext.py
  mlir/test/python/dialects/scf.py

Index: mlir/test/python/dialects/scf.py
===
--- mlir/test/python/dialects/scf.py
+++ mlir/test/python/dialects/scf.py
@@ -82,3 +82,58 @@
 # CHECK:   iter_args(%{{.*}} = %[[ARGS]]#0, %{{.*}} = %[[ARGS]]#1)
 # CHECK: scf.yield %{{.*}}, %{{.*}}
 # CHECK:   return
+
+
+@constructAndPrintInModule
+def testIfWithoutElse():
+  bool = IntegerType.get_signless(1)
+  i32 = IntegerType.get_signless(32)
+
+  @builtin.FuncOp.from_py_func(bool)
+  def simple_if(cond):
+if_op = scf.IfOp(cond)
+with InsertionPoint(if_op.then_block):
+  one = arith.ConstantOp(i32, 1)
+  add = arith.AddIOp(one, one)
+  scf.YieldOp([])
+return
+
+
+# CHECK: func @simple_if(%[[ARG0:.*]]: i1)
+# CHECK: scf.if %[[ARG0:.*]]
+# CHECK:   %[[ONE:.*]] = arith.constant 1
+# CHECK:   %[[ADD:.*]] = arith.addi %[[ONE]], %[[ONE]]
+# CHECK: return
+
+
+@constructAndPrintInModule
+def testIfWithElse():
+  bool = IntegerType.get_signless(1)
+  i32 = IntegerType.get_signless(32)
+
+  @builtin.FuncOp.from_py_func(bool)
+  def simple_if_else(cond):
+if_op = scf.IfOp(cond, [i32, i32], hasElse=True)
+with InsertionPoint(if_op.then_block):
+  x_true = arith.ConstantOp(i32, 0)
+  y_true = arith.ConstantOp(i32, 1)
+  scf.YieldOp([x_true, y_true])
+with InsertionPoint(if_op.else_block):
+  x_false = arith.ConstantOp(i32, 2)
+  y_false = arith.ConstantOp(i32, 3)
+  scf.YieldOp([x_false, y_false])
+add = arith.AddIOp(if_op.results[0], if_op.results[1])
+return
+
+
+# CHECK: func @simple_if_else(%[[ARG0:.*]]: i1)
+# CHECK: %[[RET:.*]]:2 = scf.if %[[ARG0:.*]]
+# CHECK:   %[[ZERO:.*]] = arith.constant 0
+# CHECK:   %[[ONE:.*]] = arith.constant 1
+# CHECK:   scf.yield %[[ZERO]], %[[ONE]]
+# CHECK: } else {
+# CHECK:   %[[TWO:.*]] = arith.constant 2
+# CHECK:   %[[THREE:.*]] = arith.constant 3
+# CHECK:   scf.yield %[[TWO]], %[[THREE]]
+# CHECK: arith.addi %[[RET]]#0, %[[RET]]#1
+# CHECK: return
Index: mlir/python/mlir/dialects/_scf_ops_ext.py
===
--- mlir/python/mlir/dialects/_scf_ops_ext.py
+++ mlir/python/mlir/dialects/_scf_ops_ext.py
@@ -64,3 +64,44 @@
 To obtain the loop-carried operands, use `iter_args`.
 """
 return self.body.arguments[1:]
+
+
+class IfOp:
+  """Specialization for the SCF if op class."""
+
+  def __init__(self,
+   cond,
+   results_=[],
+   *,
+   hasElse=False,
+   loc=None,
+   ip=None):
+"""Creates an SCF `if` operation.
+
+- `cond` is a MLIR value of 'i1' type to determine which regions of code will be executed.
+- `hasElse` determines whether the if operation has the else branch.
+"""
+operands = []
+operands.append(cond)
+results = []
+results.extend(results_)
+super().__init__(
+self.build_generic(
+regions=2,
+results=results,
+operands=operands,
+loc=loc,
+ip=ip))
+self.regions[0].blocks.append(*[])
+if hasElse:
+self.regions[1].blocks.append(*[])
+
+  @property
+  def then_block(self):
+"""Returns the then block of the if operation."""
+return self.regions[0].blocks[0]
+
+  @property
+  def else_block(self):
+"""Returns the else block of the if operation."""
+return self.regions[1].blocks[0]
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Seems like there is a local base commit in your repository: you're not 
uploading a diff that applies on HEAD.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121076/new/

https://reviews.llvm.org/D121076

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121076: [MLIR][Python] Add SCFIfOp Python binding

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Can you rebase? Your patch does not apply apparently


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121076/new/

https://reviews.llvm.org/D121076

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120375: Trim unnecessary component/library dependencies.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Please expand the description and include how you figured this out and how do 
we know it is actually correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120375/new/

https://reviews.llvm.org/D120375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D72404#3307623 , @aykevl wrote:

> So, should all passes just look at the `optsize` and `minsize` attributes 
> instead of the `SizeLevel`? In other words, should 
> `PassManagerBuilder.SizeLevel` be removed and should passes only look at 
> function attributes instead of `SizeLevel`? Because at the moment, it's a 
> weird mix of both. IMHO size level should either all go via function 
> attributes or via a flag, not something in between as it is now.

I agree, I don't know (other than history) why we couldn't move towards 
removing `PassManagerBuilder.SizeLevel`?

> Also, if size level is done via function attributes, why not optimization 
> level? There is already `optnone`. I'm not saying that's better, but right 
> now I don't see the logic in this whole system.

Despite what gcc and clang exposes to their users, at the LLVM level we don't 
have a single dimension on which to put `O1/O2/O3` compared to `Os/Oz`. These 
also may not make sense for every single compiler out-there: `O1/O2/O3` for 
clang may not be the right pass pipeline for my proprietary shader compiler.
Another reason why O1 /O2 
/O3 
 are not making much such to be in 
the IR is that the IR is intended to be stored and reloaded any time in the 
middle of pipeline. LTO is an example of this, so we don't really want to store 
the "list of pass to run" in the IR.
Finally, we don't want to (or we can't really...) teach passes about whether 
they should execute during O1 /O2 
/O3 
, while `optnone` is just a "fuse" 
to disable them all. It is also convenient to have `optnone` as a function 
attribute because it allows to selectively disable the optimizer on a per 
function basis. On the other hand because of the nature of the pass pipeline, 
it can't be tweaked on a per function basis (what about Module passes?).

On the other hand the optsize/minsize are driving heuristic and can be 
orthogonal to the pass pipeline / controlled on a per-function basis and made 
available to every pass: they convey an "optimization goal" that applies to 
every pass individually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72404/new/

https://reviews.llvm.org/D72404

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119257: Revert "Re-land [LLD] Remove global state in lldCommon"

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Yes, I saw these, thanks a lot @aganea !!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119257/new/

https://reviews.llvm.org/D119257

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119257: Revert "Re-land [LLD] Remove global state in lldCommon"

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

(The discussion seems to be happening in two places: 
https://github.com/llvm/llvm-project/issues/53475 )

You can break downstream users, the relationship inside the monorepo is 
different as far as I know: I believe we will have to live with revert like the 
one above until you collaborate actively to find a supported way and move there 
though. 
Just asking "please move away from the library usage" alone won't be enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119257/new/

https://reviews.llvm.org/D119257

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D72404#3304205 , @aykevl wrote:

> I would be very interested in this patch, because for me to use ThinLTO 
> without size regressions I need to set the optimization size level in the 
> linker (`PassManagerBuilder.SizeLevel` etc).

Why can't you build the object files with `-Os` in the first place instead of 
changing the optimization level between the compilation and the linking phases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72404/new/

https://reviews.llvm.org/D72404

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118652: Cleanup header dependencies in LLVMCore

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

I'm not sure how it'd help clients of the released version of LLVM to delay, 
the development branch and the release seems fairly disconnected to me from 
this point of view.
(what can help are deprecating APIs with instructions about how to update so 
that they can update to the new API while targeting the current release)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118652/new/

https://reviews.llvm.org/D118652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini abandoned this revision.
mehdi_amini added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+   a human reader, and there's basically no place for a bug to hide. On the 
other
+   hand for non-public functions, all the call-sites are visible and the 
parameter
+   can be eliminated entirely.

aaron.ballman wrote:
> mehdi_amini wrote:
> > aaron.ballman wrote:
> > > Call sites are not always visible for protected functions, so this seems 
> > > a bit suspicious. The changes are missing test coverage for that 
> > > situation.
> > You're using `public` for "access control" while I was using the linkage 
> > aspect: my reasoning is that if a method isn't "externally visible" from 
> > the current translation unit, you see all the call-sites. This is 
> > orthogonal to public/private/protected as far as I know.
> > 
> > I am likely missing a check for "isDefinedInMainFile" (or whatever the api 
> > is called) to not flag included headers.
> > You're using public for "access control" while I was using the linkage 
> > aspect: my reasoning is that if a method isn't "externally visible" from 
> > the current translation unit, you see all the call-sites.
> 
> Oh, thanks for pointing out that I was confused! I'm not used to "public" 
> when describing linkage, usually that's "external" or "externally visible". 
> Any appetite for rewording in those terms? Something like "On the other hand, 
> for functions with internal linkage, all the call sites are visible and 
> parameters can be safely removed."
Sure: happy to reword.

(We use public/private for symbol visibility at a module level in MLIR 
unfortunately, I've been "contaminated" ;) ).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
+// CHECK-FIXES: C() {}
+  C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning

aaron.ballman wrote:
> mehdi_amini wrote:
> > aaron.ballman wrote:
> > > I think this demonstrates a bad fix -- this changes code meaning from 
> > > being a converting constructor to being a default constructor, which are 
> > > very different things.
> > Oh you're right: so we can't do it for a Ctor with a single parameter...
> > 
> > But we also can't do it for a Ctor with two parameters as it'll turn it 
> > into a converting ctor. Unless you can eliminate both parameters, in which 
> > case it is become a default ctor (which can conflict with an existing one, 
> > in which case it can be just deleted?)
> Yeah, I think we need to not transform ctors at all currently.
In this case I can fix the existing bug by disabling the fixit  (as discussed 
in D116513) and abandon this revision I think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116512/new/

https://reviews.llvm.org/D116512

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116513: [clang-tidy] Fix bugs in misc-unused-parameters for Constructors calls site

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Thanks @aaron.ballman for the review!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
 // CHECK-FIXES: C() {}
   C(int i) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning

aaron.ballman wrote:
> I think this fix is incorrect and should not be applied; it changes the 
> meaning of the interface from having a converting constructor to having a 
> default constructor. I think that needs to be optional behavior because it's 
> a pretty invasive change to apply automatically. This patch builds on top of 
> the existing incorrect behavior, but would be fine if it was only applied 
> when the option to modify constructors is enabled.
I'm not against an option, but I'd like to get to a default behavior that is 
"safe". So I guess my first patch should be to undo the transformation 
happening here in the test already.
We can either never touch any C++ constructor or try to find a conservative 
logic for it.
I mentioned in the other review that we may avoid touching a Ctor with a single 
parameter. 

Then we also can't do it for a Ctor with two parameters as it'll turn it into a 
converting ctor. Unless you can eliminate both parameters, in which case it is 
become a default ctor (which can conflict with an existing one, in which case 
it can be just deleted?)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116513/new/

https://reviews.llvm.org/D116513

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+   a human reader, and there's basically no place for a bug to hide. On the 
other
+   hand for non-public functions, all the call-sites are visible and the 
parameter
+   can be eliminated entirely.

aaron.ballman wrote:
> Call sites are not always visible for protected functions, so this seems a 
> bit suspicious. The changes are missing test coverage for that situation.
You're using `public` for "access control" while I was using the linkage 
aspect: my reasoning is that if a method isn't "externally visible" from the 
current translation unit, you see all the call-sites. This is orthogonal to 
public/private/protected as far as I know.

I am likely missing a check for "isDefinedInMainFile" (or whatever the api is 
called) to not flag included headers.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
+// CHECK-FIXES: C() {}
+  C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning

aaron.ballman wrote:
> I think this demonstrates a bad fix -- this changes code meaning from being a 
> converting constructor to being a default constructor, which are very 
> different things.
Oh you're right: so we can't do it for a Ctor with a single parameter...

But we also can't do it for a Ctor with two parameters as it'll turn it into a 
converting ctor. Unless you can eliminate both parameters, in which case it is 
become a default ctor (which can conflict with an existing one, in which case 
it can be just deleted?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116512/new/

https://reviews.llvm.org/D116512

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116513: [clang-tidy] Fix bugs in misc-unused-parameters for Constructors calls site

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini created this revision.
mehdi_amini added a reviewer: Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, xazax.hun.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

These weren't tracked and so weren't updated when applying fixes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116513

Files:
  clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -186,6 +186,7 @@
 
 void someMoreCallSites() {
   C c(42);
+// CHECK-FIXES: C c();
   c.f(1);
 // CHECK-FIXES: c.f();
   c.g(1);
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -64,8 +64,9 @@
: nullptr));
 }
 
+template 
 static FixItHint removeArgument(const MatchFinder::MatchResult ,
-const CallExpr *Call, unsigned Index) {
+const CallExprT *Call, unsigned Index) {
   return FixItHint::CreateRemoval(removeNode(
   Result, Index > 0 ? Call->getArg(Index - 1) : nullptr,
   Call->getArg(Index),
@@ -75,13 +76,20 @@
 class UnusedParametersCheck::IndexerVisitor
 : public RecursiveASTVisitor {
 public:
-  IndexerVisitor(ASTContext ) { TraverseAST(Ctx); }
+  IndexerVisitor(ASTContext ) : mgr(Ctx.getSourceManager()) {
+TraverseAST(Ctx);
+  }
 
   const std::unordered_set &
   getFnCalls(const FunctionDecl *Fn) {
 return Index[Fn->getCanonicalDecl()].Calls;
   }
 
+  const std::unordered_set &
+  getCtorCalls(const FunctionDecl *Fn) {
+return Index[Fn->getCanonicalDecl()].CtorRefs;
+  }
+
   const std::unordered_set &
   getOtherRefs(const FunctionDecl *Fn) {
 return Index[Fn->getCanonicalDecl()].OtherRefs;
@@ -97,6 +105,15 @@
 return true;
   }
 
+  bool WalkUpFromCXXConstructExpr(CXXConstructExpr *Call) {
+if (const auto *Ctor =
+dyn_cast_or_null(Call->getConstructor())) {
+  Ctor = Ctor->getCanonicalDecl();
+  Index[Ctor].CtorRefs.insert(Call);
+}
+return true;
+  }
+
   bool WalkUpFromCallExpr(CallExpr *Call) {
 if (const auto *Fn =
 dyn_cast_or_null(Call->getCalleeDecl())) {
@@ -114,8 +131,9 @@
   struct IndexEntry {
 std::unordered_set Calls;
 std::unordered_set OtherRefs;
+std::unordered_set CtorRefs;
   };
-
+  SourceManager 
   std::unordered_map Index;
 };
 
@@ -165,9 +183,14 @@
   MyDiag << removeParameter(Result, FD, ParamIndex);
 
   // Fix all call sites.
-  for (const CallExpr *Call : Indexer->getFnCalls(Function))
+  for (const CallExpr *Call : Indexer->getFnCalls(Function)) {
+if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
+  MyDiag << removeArgument(Result, Call, ParamIndex);
+  }
+  for (const CXXConstructExpr *Call : Indexer->getCtorCalls(Function)) {
 if (ParamIndex < Call->getNumArgs()) // See PR38055 for example.
   MyDiag << removeArgument(Result, Call, ParamIndex);
+  }
 }
 
 void UnusedParametersCheck::check(const MatchFinder::MatchResult ) {


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -186,6 +186,7 @@
 
 void someMoreCallSites() {
   C c(42);
+// CHECK-FIXES: C c();
   c.f(1);
 // CHECK-FIXES: c.f();
   c.g(1);
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -64,8 +64,9 @@
: nullptr));
 }
 
+template 
 static FixItHint removeArgument(const MatchFinder::MatchResult ,
-const CallExpr *Call, unsigned Index) {
+const CallExprT *Call, unsigned Index) {
   return FixItHint::CreateRemoval(removeNode(
   Result, Index > 0 ? Call->getArg(Index - 1) : nullptr,
   Call->getArg(Index),
@@ -75,13 +76,20 @@
 class UnusedParametersCheck::IndexerVisitor
 : public RecursiveASTVisitor {
 public:
-  IndexerVisitor(ASTContext ) { TraverseAST(Ctx); }
+  IndexerVisitor(ASTContext ) : mgr(Ctx.getSourceManager()) {
+TraverseAST(Ctx);
+  }
 
   const 

[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini created this revision.
mehdi_amini added a reviewer: Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, xazax.hun.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The rational to avoid applying the warning/fix in non-Strict more to
functions with an empty body is that there is "no place for a bug to hide".
However for private functions, the parameters can be entirely eliminated
and all the call sites improved.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116512

Files:
  clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -154,6 +154,10 @@
 namespace {
 class C {
 public:
+// CHECK-FIXES: C() {}
+  C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
+
   void f(int i);
 // CHECK-FIXES: void f();
   void g(int i) {;}
@@ -181,7 +185,7 @@
 void useFunction(T t);
 
 void someMoreCallSites() {
-  C c;
+  C c(42);
   c.f(1);
 // CHECK-FIXES: c.f();
   c.g(1);
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -37,6 +37,8 @@
 
When `false` (default value), the check will ignore trivially unused 
parameters,
i.e. when the corresponding function has an empty body (and in case of
-   constructors - no constructor initializers). When the function body is 
empty,
-   an unused parameter is unlikely to be unnoticed by a human reader, and
-   there's basically no place for a bug to hide.
+   constructors - no constructor initializers) and the definition is public. 
When
+   the function body is empty, an unused parameter is unlikely to be unnoticed 
by
+   a human reader, and there's basically no place for a bug to hide. On the 
other
+   hand for non-public functions, all the call-sites are visible and the 
parameter
+   can be eliminated entirely.
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -183,9 +183,9 @@
 Param->hasAttr())
   continue;
 
-// In non-strict mode ignore function definitions with empty bodies
+// In non-strict mode ignore public function definitions with empty bodies
 // (constructor initializer counts for non-empty body).
-if (StrictMode ||
+if (StrictMode || !Function->isExternallyVisible() ||
 (Function->getBody()->child_begin() !=
  Function->getBody()->child_end()) ||
 (isa(Function) &&


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -154,6 +154,10 @@
 namespace {
 class C {
 public:
+// CHECK-FIXES: C() {}
+  C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
+
   void f(int i);
 // CHECK-FIXES: void f();
   void g(int i) {;}
@@ -181,7 +185,7 @@
 void useFunction(T t);
 
 void someMoreCallSites() {
-  C c;
+  C c(42);
   c.f(1);
 // CHECK-FIXES: c.f();
   c.g(1);
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -37,6 +37,8 @@
 
When `false` (default value), the check will ignore trivially unused parameters,
i.e. when the corresponding function has an empty body (and in case of
-   constructors - no constructor initializers). When the function body is empty,
-   an unused parameter is unlikely to be unnoticed by a human reader, and
-   there's basically no place for a bug to hide.
+   constructors - no constructor initializers) and the definition is public. When
+   the function body is empty, an unused parameter is unlikely to be unnoticed by
+   a human reader, and there's basically no place for a bug to hide. On the other
+   hand for non-public functions, all the call-sites are visible and the parameter
+   can be eliminated entirely.
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp

[PATCH] D116488: Add a misc-unused-parameters.CommentOutUnusedParameters to clang-tidy

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:25
+
+  void a(int ) { /*some code that doesn't use `i`*/ }
+

jpienaar wrote:
> OOC why the extra space after the type?
The space is in the original code, the fix only touches the name (clang-format 
is expected to fix it afterward)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116488/new/

https://reviews.llvm.org/D116488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116488: Add a misc-unused-parameters.CommentOutUnusedParameters to clang-tidy

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini created this revision.
mehdi_amini added reviewers: rriddle, jpienaar, Mogball.
Herald added a subscriber: carlosgalvezp.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This option allows to eliminate parameter names instead of commenting
them out.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116488

Files:
  clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
  
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters-no-comment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters-no-comment.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters-no-comment.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s misc-unused-parameters %t -- \
+// RUN:   -config="{CheckOptions: [{key: 
misc-unused-parameters.CommentOutUnusedParameters, value: false}]}" --
+
+// Check that we eliminate parameters instead of commenting them out.
+class SomeClass {
+  static void f(int i) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void f(int ) {;}
+  static void g(int i = 1) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void g(int = 1) {;}
+  static void h(int i[]) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void h(int []) {;}
+  static void s(void (*fn)()) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning
+  // CHECK-FIXES: static void s(void (*)()) {;}
+};
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -20,6 +20,10 @@
 
   void a(int  /*i*/) { /*some code that doesn't use `i`*/ }
 
+  // or if the CommentOutUnusedParameters is set to false:
+
+  void a(int ) { /*some code that doesn't use `i`*/ }
+
 .. code-block:: c++
 
   static void staticFunctionA(int i);
@@ -40,3 +44,8 @@
constructors - no constructor initializers). When the function body is 
empty,
an unused parameter is unlikely to be unnoticed by a human reader, and
there's basically no place for a bug to hide.
+
+.. option:: misc-unused-parameters.CommentOutUnusedParameters
+
+   When `true` (default value), the suggest fix will comment out the parameter 
name
+   instead of removing them.
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -154,8 +154,11 @@
 // Note: We always add a space before the '/*' to not accidentally create
 // a '*/*' for pointer types, which doesn't start a comment. clang-format
 // will clean this up afterwards.
-MyDiag << FixItHint::CreateReplacement(
-RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
+if (Options.get("CommentOutUnusedParameters", true))
+  MyDiag << FixItHint::CreateReplacement(
+  RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
+else
+  MyDiag << FixItHint::CreateReplacement(RemovalRange, "");
 return;
   }
 


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters-no-comment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters-no-comment.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s misc-unused-parameters %t -- \
+// RUN:   -config="{CheckOptions: [{key: misc-unused-parameters.CommentOutUnusedParameters, value: false}]}" --
+
+// Check that we eliminate parameters instead of commenting them out.
+class SomeClass {
+  static void f(int i) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void f(int ) {;}
+  static void g(int i = 1) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void g(int = 1) {;}
+  static void h(int i[]) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning
+  // CHECK-FIXES: static void h(int []) {;}
+  static void s(void (*fn)()) { ; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning
+  // CHECK-FIXES: static void s(void (*)()) {;}
+};
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -20,6 +20,10 @@
 
   void a(int  /*i*/) { /*some code that doesn't use `i`*/ }
 
+  // or if the CommentOutUnusedParameters is set to 

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D114639#3162141 , @erichkeane 
wrote:

>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D114639#3162069 , @erichkeane 
wrote:

> In D114639#3162032 , @RKSimon wrote:
>
>> In D114639#3162000 , @mehdi_amini 
>> wrote:
>>
>>> In D114639#3161303 , @erichkeane 
>>> wrote:
>>>
 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D114639#3161303 , @erichkeane 
wrote:

> 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.
(I'm not against updating our toolchain to support c++17, but that's orthogonal 
to the MSVC-specific update)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114699: Fixed a memory leak in the PDLToPDLInterp RootOrderingTest.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp:37
+  ~RootOrderingTest() {
+for (int i = 0; i < 4; ++i)
+  v[i].getDefiningOp()->destroy();

bondhugula wrote:
> bondhugula wrote:
> > You need to be erasing those ops using `erase()`. 
> Nit: `unsigned` here and below.
@bondhugula unsigned is an anti-pattern (unless working with bit-fields or 
other bit-manipulation), this has been widely documented I believe, both in C++ 
standardization paper (they are stuck with size_t in the standard library) and 
with conference talks like this one: https://www.youtube.com/watch?v=yG1OZ69H_-o


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114699/new/

https://reviews.llvm.org/D114699

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D114639#3158401 , @RKSimon wrote:

>> Have you checked whether there are any bots in the lab that will need to be 
>> updated?
>
> I did find a number of bots that I think need addressing, I am intending to 
> privately email the owners but I haven't done that yet - @rnk maintains the 
> sanitizer-windows and windows-gcebot2 machines, but there are others.

This bot as well: https://lab.llvm.org/buildbot/#/builders/13 ; adding 
@stella.stamenova here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114639/new/

https://reviews.llvm.org/D114639

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114502: File Reorganization changes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/lib/ExecutionEngine/CMakeLists.txt:153
+  set(CMAKE_MODULE_PATH "${HIP_PATH}/lib/cmake/hip" ${CMAKE_MODULE_PATH})
   find_package(HIP)
   if (NOT HIP_FOUND)

mehdi_amini wrote:
> Is MLIR really coupled to Clang here? That seems suspicious to me.
In particular, this makes MLIR sensitive to the version actually installed on 
the machine, which is independent from whatever clang is in the repo. This 
seems undesirable, what am I missing here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114502/new/

https://reviews.llvm.org/D114502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114502: File Reorganization changes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/lib/ExecutionEngine/CMakeLists.txt:153
+  set(CMAKE_MODULE_PATH "${HIP_PATH}/lib/cmake/hip" ${CMAKE_MODULE_PATH})
   find_package(HIP)
   if (NOT HIP_FOUND)

Is MLIR really coupled to Clang here? That seems suspicious to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114502/new/

https://reviews.llvm.org/D114502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114394: Compile-time computation of string attribute hashes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: llvm/lib/IR/Attributes.cpp:125
   FoldingSetNodeID ID;
-  ID.AddString(Kind);
+  ID.AddString(Kind.value());
   if (!Val.empty()) ID.AddString(Val);

Carrying over my comment from the original revision: it seem that you're ever 
only using the StringRef from the Kind in this function.
If so changing the API to use an AttributeKey seems pessimizing to me?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114394/new/

https://reviews.llvm.org/D114394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

The patch is pretty large, any chance you could split in two? First the change 
introducing the AttributeKey and changing the API, and then updating all the 
callees separately as a follow up?




Comment at: llvm/include/llvm/IR/Attributes.h:84
+  }
+};
 

This whole code deserves documentation I think.



Comment at: llvm/lib/IR/Attributes.cpp:125
   FoldingSetNodeID ID;
-  ID.AddString(Kind);
+  ID.AddString(Kind.value());
   if (!Val.empty()) ID.AddString(Val);

Seems like this method does not use anything than the `StringRef` inside the 
Kind, why change it to take an `AttributeKey`? Won't you eagerly compute the 
hash even if not needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109977: LLVM Driver Multicall tool

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

That's pretty nice! Have you thought about looking into a lit option (triggered 
by a cmake flag maybe) that would change the substitution for the tools that 
are enabled by llvm-driver to use the latter instead when running the tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109977/new/

https://reviews.llvm.org/D109977

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109167: Try to unbreak Win build differently after 973519826edb76

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> @goncharov @mehdi_amini is there a way to change the way the phab UI or the 
> pre-merge checks work here to make failing pre-merge checks clearer? This 
> isn't the first time I've seen someone be very confused about this UI

I asked the same thing a few days ago and @goncharov said something was cooking 
in this area!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109167/new/

https://reviews.llvm.org/D109167

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> the original discussion that spawned the (not-yet-accepted, as it says in its 
> opening sentence) linked proposal around variable naming.

+1 with David again: if the proposal gets accepted, then this patch makes sense 
to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108265/new/

https://reviews.llvm.org/D108265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

+1 on arguments from @dblaikie : I'm not sure I understand the motivation for 
this change right now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108265/new/

https://reviews.llvm.org/D108265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107349: [Matrix] Overload stride arg in matrix.columnwise.load/store.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Revert to unbreak bots (like this one : 
https://lab.llvm.org/buildbot/#/builders/13/builds/10930 )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107349/new/

https://reviews.llvm.org/D107349

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Something else I'm not sure about is how it works across DSOs: when each LLVM 
library is linked into its own shared library, is the dynamic linker creating 
this array when loading the libraries?
(I'm starting to doubt about how this would even work on windows)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

That's interesting!

I'm not sure how to get there though: a complete solution should be able to 
"degrade" to global constructor when the platform-specific logic isn't 
available (unless we're confident that no such environment can exist?).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG42f588f39c5c: Use ManagedStatic and lazy initialization of 
cl::opt in libSupport to make it… (authored by mehdi_amini).

Changed prior to commit:
  https://reviews.llvm.org/D105959?vs=359196=359201#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

Files:
  clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/Windows/Signals.inc
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == ::GeneralCategory;
+  return Cat == ::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
===
--- 

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D105959#2882099 , @bondhugula 
wrote:

> This is a really welcome change! Multiple registration issues were really an 
> inconvenience - I had no clue this was the pattern to use to fix it. Thanks!

To be fair: we can do it transparently only for libSupport, as it contains the 
entry point for command line parsing it can explicitly trigger the registration 
of the options for itself. Other libraries in LLVM don't have this luxury...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini updated this revision to Diff 359196.
mehdi_amini added a comment.

Fix windows build


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

Files:
  clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/Windows/Signals.inc
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == ::GeneralCategory;
+  return Cat == ::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
===
--- llvm/unittests/Support/ARMAttributeParser.cpp
+++ llvm/unittests/Support/ARMAttributeParser.cpp
@@ -48,7 +48,7 @@
 }
 
 bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, 

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini updated this revision to Diff 359191.
mehdi_amini marked 7 inline comments as done.
mehdi_amini added a comment.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added projects: clang, clang-tools-extra.

Address comment and fix build errors


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

Files:
  clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  llvm/docs/CommandLine.rst
  llvm/include/llvm/Support/ARMAttributeParser.h
  llvm/include/llvm/Support/ARMBuildAttributes.h
  llvm/include/llvm/Support/CommandLine.h
  llvm/include/llvm/Support/RISCVAttributeParser.h
  llvm/include/llvm/Support/RISCVAttributes.h
  llvm/include/llvm/Support/ScopedPrinter.h
  llvm/include/llvm/Support/WithColor.h
  llvm/lib/Support/ARMBuildAttrs.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/CommandLine.cpp
  llvm/lib/Support/Debug.cpp
  llvm/lib/Support/DebugCounter.cpp
  llvm/lib/Support/DebugOptions.h
  llvm/lib/Support/ELFAttributeParser.cpp
  llvm/lib/Support/GraphWriter.cpp
  llvm/lib/Support/RISCVAttributes.cpp
  llvm/lib/Support/RandomNumberGenerator.cpp
  llvm/lib/Support/Signals.cpp
  llvm/lib/Support/Statistic.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/TypeSize.cpp
  llvm/lib/Support/WithColor.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
  llvm/unittests/Support/ARMAttributeParser.cpp
  llvm/unittests/Support/CommandLineTest.cpp
  llvm/unittests/Support/RISCVAttributeParserTest.cpp

Index: llvm/unittests/Support/RISCVAttributeParserTest.cpp
===
--- llvm/unittests/Support/RISCVAttributeParserTest.cpp
+++ llvm/unittests/Support/RISCVAttributeParserTest.cpp
@@ -49,7 +49,7 @@
 }
 
 static bool testTagString(unsigned Tag, const char *name) {
-  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::RISCVAttributeTags)
+  return ELFAttrs::attrTypeAsString(Tag, RISCVAttrs::getRISCVAttributeTags())
  .str() == name;
 }
 
Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -110,7 +110,7 @@
   ASSERT_NE(Retrieved->Categories.end(),
 find_if(Retrieved->Categories,
 [&](const llvm::cl::OptionCategory *Cat) {
-  return Cat == ::GeneralCategory;
+  return Cat == ::getGeneralCategory();
 }))
   << "Incorrect default option category.";
 
@@ -152,10 +152,10 @@
 
 TEST(CommandLineTest, UseMultipleCategories) {
   StackOption TestOption2("test-option2", cl::cat(TestCategory),
-   cl::cat(cl::GeneralCategory),
-   cl::cat(cl::GeneralCategory));
+   cl::cat(cl::getGeneralCategory()),
+   cl::cat(cl::getGeneralCategory()));
 
-  // Make sure cl::GeneralCategory wasn't added twice.
+  // Make sure cl::getGeneralCategory() wasn't added twice.
   ASSERT_EQ(TestOption2.Categories.size(), 2U);
 
   ASSERT_NE(TestOption2.Categories.end(),
@@ -166,9 +166,9 @@
   << "Failed to assign Option Category.";
   ASSERT_NE(TestOption2.Categories.end(),
 find_if(TestOption2.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to assign General Category.";
 
   cl::OptionCategory AnotherCategory("Additional test Options", "Description");
@@ -176,9 +176,9 @@
   cl::cat(AnotherCategory));
   ASSERT_EQ(TestOption.Categories.end(),
 find_if(TestOption.Categories,
- [&](const llvm::cl::OptionCategory *Cat) {
-   return Cat == ::GeneralCategory;
- }))
+[&](const llvm::cl::OptionCategory *Cat) {
+  return Cat == ::getGeneralCategory();
+}))
   << "Failed to remove General Category.";
   ASSERT_NE(TestOption.Categories.end(),
 find_if(TestOption.Categories,
Index: llvm/unittests/Support/ARMAttributeParser.cpp
===
--- llvm/unittests/Support/ARMAttributeParser.cpp
+++ 

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: llvm/lib/Support/CMakeLists.txt:7
+# ManagedStatic can be used to enable lazy-initialization of globals.
+add_flag_if_supported("-Werror=global-constructors" WERROR_GLOBAL_CONSTRUCTOR)
+

MaskRay wrote:
> Perhaps move this to a separate change.
Sure, I can commit this separately.



Comment at: llvm/lib/Support/Debug.cpp:96
 //until program termination.
-static cl::opt
-DebugBufferSize("debug-buffer-size",
-cl::desc("Buffer the last N characters of debug output "
- "until program termination. "
- "[default 0 -- immediate print-out]"),
-cl::Hidden,
-cl::init(0));
+struct CreateDebugBufferSize {
+  static void *call() {

jpienaar wrote:
> Why not in anonymous namespace too? (consistenly everywhere)
I think it is already?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105959/new/

https://reviews.llvm.org/D105959

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69498: IR: Invert convergent attribute handling

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

What matter isn't "convergent" in itself, it is how it restricts 
transformations: if you know that all the control flow is always uniform, are 
there still restriction on any transformation in presence of convergent 
instructions?

If not, then the "target approach" seems nice: as @foad and @sameerds noted we 
just need to have a guarantee on these targets about the uniformity of the 
branches, it does not require to change the convergent semantics on a target 
basis I think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69498/new/

https://reviews.llvm.org/D69498

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95766: [Branch-Rename] Fix some links

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.h:20
 // Check for underscores in the names of googletest tests, per
-// 
https://github.com/google/googletest/blob/master/googletest/docs/faq.md#why-should-test-suite-names-and-test-names-not-contain-underscore
 ///

You removed the wrong word here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95766/new/

https://reviews.llvm.org/D95766

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91980: [OpenMP] Add initial support for `omp [begin/end] assumes`

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+};
+

This is broken on gcc-5: 
```
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101:1: error: could not 
convert '(const char*)"ext_"' from 'const char*' to 'llvm::StringLiteral'
 };
 ^
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91980/new/

https://reviews.llvm.org/D91980

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Clang is crashing after this change when bootstrapping and building 
lib/Support/CMakeFiles/LLVMSupport.dir/Hashing.cpp.o
(our log if it helps: 
https://buildkite.com/mlir/mlir-core/builds/10003#80a5b6a9-d8d9-4420-ab2a-c24f398bc5d4
 )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91488/new/

https://reviews.llvm.org/D91488

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87981: [X86] AMX programming model.

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

(Drive by comments)




Comment at: llvm/include/llvm/CodeGen/TileShapeInfo.h:27
+
+class ShapeT {
+public:

Can you document the class?



Comment at: llvm/lib/Target/X86/X86PreTileConfig.cpp:30
+
+class X86PreTileConfig : public MachineFunctionPass {
+  // context

Can you document this as well?



Comment at: llvm/lib/Target/X86/X86TileConfig.cpp:34
+
+class X86TileConfig : public MachineFunctionPass {
+  // context

Can you document this pass?



Comment at: llvm/lib/Target/X86/X86TileConfig.cpp:223
+  LLVM_DEBUG(dbgs() << "** TILE REGISTER CONFIGURE**\n"
+<< "** Function: " << mf.getName() << '\n');
+  MF = 

Is this usual for MachinePasses to do this? In general the pass manager can 
display the scheduling when debugging


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87981/new/

https://reviews.llvm.org/D87981

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84293: Add an assertion in SmallVector::push_back()

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:10811-10813
+Ops.reserve(Ops.size() + 1);
 Ops.push_back(Ops[0]);
 Ops.erase(Ops.begin());

njames93 wrote:
> Probably a little off topic, but shouldn't this be refactored as a rotate. 
> Has the advantage that it will never allocate.
> 
Well spotted! I didn't even think about what the code was doing and fixed it 
mechanically...

I pushed this separately: 
https://github.com/llvm/llvm-project/commit/42e88bd6b185


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84293/new/

https://reviews.llvm.org/D84293

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84293: Add an assertion in SmallVector::push_back()

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c196bbc6bd8: Add an assertion in SmallVector::push_back() 
(authored by mehdi_amini).

Changed prior to commit:
  https://reviews.llvm.org/D84293?vs=305279=305281#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84293/new/

https://reviews.llvm.org/D84293

Files:
  llvm/include/llvm/ADT/SmallVector.h
  llvm/include/llvm/MC/MCInst.h


Index: llvm/include/llvm/MC/MCInst.h
===
--- llvm/include/llvm/MC/MCInst.h
+++ llvm/include/llvm/MC/MCInst.h
@@ -181,7 +181,7 @@
   MCOperand (unsigned i) { return Operands[i]; }
   unsigned getNumOperands() const { return Operands.size(); }
 
-  void addOperand(const MCOperand ) { Operands.push_back(Op); }
+  void addOperand(const MCOperand Op) { Operands.push_back(Op); }
 
   using iterator = SmallVectorImpl::iterator;
   using const_iterator = SmallVectorImpl::const_iterator;
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -136,6 +136,13 @@
 this->Size = this->Capacity = 0; // FIXME: Setting Capacity to 0 is 
suspect.
   }
 
+  void assertSafeToPush(const void *Elt) {
+assert(
+(Elt < begin() || Elt >= end() || this->size() < this->capacity()) &&
+"Attempting to push_back to the vector an element of the vector 
without"
+" enough space reserved");
+  }
+
 public:
   using size_type = size_t;
   using difference_type = ptrdiff_t;
@@ -251,6 +258,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(Elt);
@@ -258,6 +266,7 @@
   }
 
   void push_back(T &) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(::std::move(Elt));
@@ -353,6 +362,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 memcpy(reinterpret_cast(this->end()), , sizeof(T));


Index: llvm/include/llvm/MC/MCInst.h
===
--- llvm/include/llvm/MC/MCInst.h
+++ llvm/include/llvm/MC/MCInst.h
@@ -181,7 +181,7 @@
   MCOperand (unsigned i) { return Operands[i]; }
   unsigned getNumOperands() const { return Operands.size(); }
 
-  void addOperand(const MCOperand ) { Operands.push_back(Op); }
+  void addOperand(const MCOperand Op) { Operands.push_back(Op); }
 
   using iterator = SmallVectorImpl::iterator;
   using const_iterator = SmallVectorImpl::const_iterator;
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -136,6 +136,13 @@
 this->Size = this->Capacity = 0; // FIXME: Setting Capacity to 0 is suspect.
   }
 
+  void assertSafeToPush(const void *Elt) {
+assert(
+(Elt < begin() || Elt >= end() || this->size() < this->capacity()) &&
+"Attempting to push_back to the vector an element of the vector without"
+" enough space reserved");
+  }
+
 public:
   using size_type = size_t;
   using difference_type = ptrdiff_t;
@@ -251,6 +258,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(Elt);
@@ -258,6 +266,7 @@
   }
 
   void push_back(T &) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(::std::move(Elt));
@@ -353,6 +362,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 memcpy(reinterpret_cast(this->end()), , sizeof(T));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84293: Add an assertion in SmallVector::push_back()

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini updated this revision to Diff 305279.
mehdi_amini added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix one clang instance failing this assertion


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84293/new/

https://reviews.llvm.org/D84293

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  llvm/include/llvm/ADT/SmallVector.h
  llvm/include/llvm/MC/MCInst.h


Index: llvm/include/llvm/MC/MCInst.h
===
--- llvm/include/llvm/MC/MCInst.h
+++ llvm/include/llvm/MC/MCInst.h
@@ -181,7 +181,7 @@
   MCOperand (unsigned i) { return Operands[i]; }
   unsigned getNumOperands() const { return Operands.size(); }
 
-  void addOperand(const MCOperand ) { Operands.push_back(Op); }
+  void addOperand(const MCOperand Op) { Operands.push_back(Op); }
 
   using iterator = SmallVectorImpl::iterator;
   using const_iterator = SmallVectorImpl::const_iterator;
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -136,6 +136,13 @@
 this->Size = this->Capacity = 0; // FIXME: Setting Capacity to 0 is 
suspect.
   }
 
+  void assertSafeToPush(const void *Elt) {
+assert(
+(Elt < begin() || Elt >= end() || this->size() < this->capacity()) &&
+"Attempting to push_back to the vector an element of the vector 
without"
+" enough space reserved");
+  }
+
 public:
   using size_type = size_t;
   using difference_type = ptrdiff_t;
@@ -251,6 +258,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(Elt);
@@ -258,6 +266,7 @@
   }
 
   void push_back(T &) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(::std::move(Elt));
@@ -353,6 +362,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 memcpy(reinterpret_cast(this->end()), , sizeof(T));
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -10808,6 +10808,7 @@
   }
   case NEON::BI__builtin_neon_vst3_v:
   case NEON::BI__builtin_neon_vst3q_v: {
+Ops.reserve(Ops.size() + 1);
 Ops.push_back(Ops[0]);
 Ops.erase(Ops.begin());
 llvm::Type *Tys[2] = { VTy, Ops[3]->getType() };


Index: llvm/include/llvm/MC/MCInst.h
===
--- llvm/include/llvm/MC/MCInst.h
+++ llvm/include/llvm/MC/MCInst.h
@@ -181,7 +181,7 @@
   MCOperand (unsigned i) { return Operands[i]; }
   unsigned getNumOperands() const { return Operands.size(); }
 
-  void addOperand(const MCOperand ) { Operands.push_back(Op); }
+  void addOperand(const MCOperand Op) { Operands.push_back(Op); }
 
   using iterator = SmallVectorImpl::iterator;
   using const_iterator = SmallVectorImpl::const_iterator;
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -136,6 +136,13 @@
 this->Size = this->Capacity = 0; // FIXME: Setting Capacity to 0 is suspect.
   }
 
+  void assertSafeToPush(const void *Elt) {
+assert(
+(Elt < begin() || Elt >= end() || this->size() < this->capacity()) &&
+"Attempting to push_back to the vector an element of the vector without"
+" enough space reserved");
+  }
+
 public:
   using size_type = size_t;
   using difference_type = ptrdiff_t;
@@ -251,6 +258,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(Elt);
@@ -258,6 +266,7 @@
   }
 
   void push_back(T &) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 ::new ((void*) this->end()) T(::std::move(Elt));
@@ -353,6 +362,7 @@
 
 public:
   void push_back(const T ) {
+this->assertSafeToPush();
 if (LLVM_UNLIKELY(this->size() >= this->capacity()))
   this->grow();
 memcpy(reinterpret_cast(this->end()), , sizeof(T));
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -10808,6 +10808,7 @@
   }
   case NEON::BI__builtin_neon_vst3_v:
   case NEON::BI__builtin_neon_vst3q_v: {
+Ops.reserve(Ops.size() + 1);
 Ops.push_back(Ops[0]);
 Ops.erase(Ops.begin());
 llvm::Type *Tys[2] = { VTy, 

[PATCH] D82860: Port ObjCMTAction to new option parsing system

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Reverted in f917356f9ce0 
 ; I 
suspect a `static constexpr` in a class missing a definition out of class 
(required pre-c++17).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82860/new/

https://reviews.llvm.org/D82860

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83088: Introduce CfgTraits abstraction

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D83088#2350287 , @nhaehnle wrote:

> Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have 
> a general rule that patches can be reverted if they're obviously broken (e.g. 
> build bot problems) or clearly violate some other standard. This is a good 
> rule, but it doesn't apply here. If you think it does, please state your case 
> in the email thread that I've started on llvm-dev for this very purpose. Just 
> one thing:

I strongly disagree: the bar for post-review commit is not lower than 
pre-commit review.

Again: please revert when someone has a concern, including a *design* concern 
with you patch.

> P.S.: It's easy to miss on Phabricator, but there is already a long stack of 
> patches which build on this

(this is part of my previous point).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83088/new/

https://reviews.llvm.org/D83088

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83088: Introduce CfgTraits abstraction

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D83088#2347111 , @arsenm wrote:

> In D83088#2346322 , @mehdi_amini 
> wrote:
>
>> In D83088#2345540 , @nhaehnle wrote:
>>
>>> David, I don't think this is appropriate here. Let's take the discussion to 
>>> llvm-dev.
>>
>> Seems like David asked to revert in the meantime?
>
> -1 to reverting, which will just make the history messier with no tangible 
> benefit

This is the usual LLVM policy I believe: someone expressed a concern and ask to 
revert. We revert and discuss first.
So again: please revert.

The messier history is not an argument: we revert so many times for any random 
bot failures already, and our contribution guidelines still tell people to push 
a "fake commit" with a whitespace change to test their access.

I also see tangile benefits:

- we don't start building dependencies on newly introduced API making a revert 
more difficult later.
- the burden of convincing of the approach is on the patch author, reverting is 
forcing the discussion here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83088/new/

https://reviews.llvm.org/D83088

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83088: Introduce CfgTraits abstraction

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D83088#2345540 , @nhaehnle wrote:

> David, I don't think this is appropriate here. Let's take the discussion to 
> llvm-dev.

Seems like David asked to revert in the meantime?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83088/new/

https://reviews.llvm.org/D83088

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85596: [Docs] Fix --print-supported-cpus option rendering

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0fb2203cd6c2: [Docs] Fix --print-supported-cpus option 
rendering (authored by tmfink, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85596/new/

https://reviews.llvm.org/D85596

Files:
  clang/docs/CommandGuide/clang.rst


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -338,12 +338,12 @@
 .. option:: --print-supported-cpus
 
   Print out a list of supported processors for the given target (specified
-  through --target= or -arch ). If no target is
-  specified, the system default target will be used.
+  through ``--target=`` or :option:`-arch` ). 
If no
+  target is specified, the system default target will be used.
 
 .. option:: -mcpu=?, -mtune=?
 
-  Aliases of --print-supported-cpus
+  Acts as an alias for :option:`--print-supported-cpus`.
 
 .. option:: -march=
 


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -338,12 +338,12 @@
 .. option:: --print-supported-cpus
 
   Print out a list of supported processors for the given target (specified
-  through --target= or -arch ). If no target is
-  specified, the system default target will be used.
+  through ``--target=`` or :option:`-arch` ). If no
+  target is specified, the system default target will be used.
 
 .. option:: -mcpu=?, -mtune=?
 
-  Aliases of --print-supported-cpus
+  Acts as an alias for :option:`--print-supported-cpus`.
 
 .. option:: -march=
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Overall that would likely work for XLA. Something I'd like to mention though in 
response to:

> The veclib type is also tied to the accepted values for -fveclib, which is a 
> list of supported lib,

`-fveclib` is a Clang thing, it shouldn't limit what LLVM does. Of course LLVM 
needs to support Clang, but does not have to inherit the limitation of map 1:1 
to Clang UI.
In particular as a library, it isn't clear why we would make the choice to 
write LLVM VecLib support this way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77925/new/

https://reviews.llvm.org/D77925

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

I rather have a non closed list of veclib: if you just have a map keyed by 
string instead of an enum it should just work out?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77925/new/

https://reviews.llvm.org/D77925

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84691: [CMake] Move find_package(ZLIB) to LLVMConfig

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84691/new/

https://reviews.llvm.org/D84691



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/examples/standalone/CMakeLists.txt:35
+endif()
+
 include(TableGen)

mehdi_amini wrote:
> I am a bit unsure that it is desirable that it is desirable to need this 
> change?
> This requires every single user of LLVM to do this. Is there a way this can 
> be done in the call to `find_package(LLVM)` instead?
(Doc for the usual way to integrate LLVM: 
https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/examples/standalone/CMakeLists.txt:35
+endif()
+
 include(TableGen)

I am a bit unsure that it is desirable that it is desirable to need this change?
This requires every single user of LLVM to do this. Is there a way this can be 
done in the call to `find_package(LLVM)` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

It seems like this pass was added in lib/Transforms but does not have any 
unit-tests (lit tests) provided. It isn't even linked into `opt`. As it is an 
LLVM IR pass it should be tested as such I believe. Can you provide tests for 
this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65761/new/

https://reviews.llvm.org/D65761



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81422: Change filecheck default to dump input on failure

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked an inline comment as done.
mehdi_amini added a comment.

In D81422#2096643 , @arsenm wrote:

> I would also expect a simple command line flag to llvm-lit to be able to 
> control this, rather than having to set an environment variable


Would you like a lit command line flag that set the environment variable? I 
think that's easily doable




Comment at: mlir/test/mlir-tblgen/op-format-spec.td:1
-// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I 
%S/../../include %s -o=%t 2>&1 | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I 
%S/../../include %s -o=%t 2>&1 | FileCheck %s
 

arsenm wrote:
> All of these MLIR tests are microscopic and I don't think this is a 
> representative sample across all the projects. Most testcases are 
> significantly larger, and have hundreds if not thousands of lines of output
Well, if this can act as an extra incentive to break-up such large tests, it 
see it as a win: debugging failing patterns in the middle of such a large test 
output is not nice regardless.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81422: Change filecheck default to dump input on failure

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D81422#2093360 , @arsenm wrote:

> The amount of context printed previously was good enough for development use. 
> If I ever can't figure it out from the diff, I want to look at the output 
> completely separate from the terminal.


I have the totally opposite experience: I can't do any debugging efficiently 
without this option.  Empirically, most new-comers to writing tests with 
FileCheck (in MLIR, TensorFlow, XLA, IREE, etc.) have been manually adding 
-dump-input-on-failure hard-coded into the test themselves, which is a pretty 
good indication.

Having something contextual around the failures would work for me as well: but 
the previous situation was just not usable for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81422/new/

https://reviews.llvm.org/D81422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81045: [LLVM] Change isa<> to a variadic function template

[Mehdi AMINI via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG72d20b9604f6: [LLVM] Change isa to a variadic 
function template (authored by jurahul, committed by mehdi_amini).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81045/new/

https://reviews.llvm.org/D81045

Files:
  clang/include/clang/AST/DeclBase.h
  llvm/include/llvm/Support/Casting.h
  llvm/lib/Analysis/ScalarEvolution.cpp
  llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp


Index: llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
===
--- llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
+++ llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
@@ -220,7 +220,7 @@
 auto Defs = findAllDefs(U);
 
 // If the values are all Constants or Arguments, don't bother
-if (llvm::none_of(Defs, isa))
+if (llvm::none_of(Defs, [](Value *V) { return isa(V); }))
   return false;
 
 // Presently, we only know how to handle PHINode, Constant, Arguments and
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -3860,7 +3860,8 @@
   if (I != HasRecMap.end())
 return I->second;
 
-  bool FoundAddRec = SCEVExprContains(S, isa);
+  bool FoundAddRec =
+  SCEVExprContains(S, [](const SCEV *S) { return isa(S); 
});
   HasRecMap.insert({S, FoundAddRec});
   return FoundAddRec;
 }
@@ -11201,8 +11202,9 @@
 // Returns true when one of the SCEVs of Terms contains a SCEVUnknown 
parameter.
 static inline bool containsParameters(SmallVectorImpl ) {
   for (const SCEV *T : Terms)
-if (SCEVExprContains(T, isa))
+if (SCEVExprContains(T, [](const SCEV *S) { return isa(S); }))
   return true;
+
   return false;
 }
 
Index: llvm/include/llvm/Support/Casting.h
===
--- llvm/include/llvm/Support/Casting.h
+++ llvm/include/llvm/Support/Casting.h
@@ -132,24 +132,30 @@
   }
 };
 
-// isa - Return true if the parameter to the template is an instance of the
-// template type argument.  Used like this:
+// isa - Return true if the parameter to the template is an instance of one
+// of the template type arguments.  Used like this:
 //
 //  if (isa(myVal)) { ... }
+//  if (isa(myVal)) { ... }
 //
 template  LLVM_NODISCARD inline bool isa(const Y ) {
   return isa_impl_wrap::SimpleType>::doit(Val);
 }
 
+template 
+LLVM_NODISCARD inline bool isa(const Y ) {
+  return isa(Val) || isa(Val);
+}
+
 // isa_and_nonnull - Functionally identical to isa, except that a null value
 // is accepted.
 //
-template 
+template 
 LLVM_NODISCARD inline bool isa_and_nonnull(const Y ) {
   if (!Val)
 return false;
-  return isa(Val);
+  return isa(Val);
 }
 
 
//===--===//
Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -518,7 +518,7 @@
 if (!HasAttrs) return;
 
 AttrVec  = getAttrs();
-Vec.erase(std::remove_if(Vec.begin(), Vec.end(), isa), 
Vec.end());
+llvm::erase_if(Vec, [](Attr *A) { return isa(A); });
 
 if (Vec.empty())
   HasAttrs = false;


Index: llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
===
--- llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
+++ llvm/lib/Target/PowerPC/PPCBoolRetToInt.cpp
@@ -220,7 +220,7 @@
 auto Defs = findAllDefs(U);
 
 // If the values are all Constants or Arguments, don't bother
-if (llvm::none_of(Defs, isa))
+if (llvm::none_of(Defs, [](Value *V) { return isa(V); }))
   return false;
 
 // Presently, we only know how to handle PHINode, Constant, Arguments and
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -3860,7 +3860,8 @@
   if (I != HasRecMap.end())
 return I->second;
 
-  bool FoundAddRec = SCEVExprContains(S, isa);
+  bool FoundAddRec =
+  SCEVExprContains(S, [](const SCEV *S) { return isa(S); });
   HasRecMap.insert({S, FoundAddRec});
   return FoundAddRec;
 }
@@ -11201,8 +11202,9 @@
 // Returns true when one of the SCEVs of Terms contains a SCEVUnknown parameter.
 static inline bool containsParameters(SmallVectorImpl ) {
   for (const SCEV *T : Terms)
-if (SCEVExprContains(T, isa))
+if (SCEVExprContains(T, [](const SCEV *S) { return isa(S); }))
   return true;
+
   return false;
 }
 
Index: llvm/include/llvm/Support/Casting.h
===
--- llvm/include/llvm/Support/Casting.h
+++ llvm/include/llvm/Support/Casting.h
@@ -132,24 +132,30 @@
   }
 };
 
-// isa - Return true