[clang] c5fb73c - [APFloat] Add recoverable string parsing errors to APFloat

[Ehud Katz via cfe-commits]

Author: Ehud Katz
Date: 2020-01-06T10:09:01+02:00
New Revision: c5fb73c5d1b3f1adb77d99fc85c594b48bff08f9

URL: 
https://github.com/llvm/llvm-project/commit/c5fb73c5d1b3f1adb77d99fc85c594b48bff08f9
DIFF: 
https://github.com/llvm/llvm-project/commit/c5fb73c5d1b3f1adb77d99fc85c594b48bff08f9.diff

LOG: [APFloat] Add recoverable string parsing errors to APFloat

Implementing the APFloat part in PR4745.

Differential Revision: https://reviews.llvm.org/D69770

Added: 


Modified: 
clang/lib/Lex/LiteralSupport.cpp
llvm/include/llvm/ADT/APFloat.h
llvm/lib/MC/MCParser/AsmParser.cpp
llvm/lib/Support/APFloat.cpp
llvm/lib/Support/StringRef.cpp
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
llvm/unittests/ADT/APFloatTest.cpp

Removed: 




diff  --git a/clang/lib/Lex/LiteralSupport.cpp 
b/clang/lib/Lex/LiteralSupport.cpp
index 2108408377fb..66309183a144 100644
--- a/clang/lib/Lex/LiteralSupport.cpp
+++ b/clang/lib/Lex/LiteralSupport.cpp
@@ -1051,7 +1051,10 @@ NumericLiteralParser::GetFloatValue(llvm::APFloat 
&Result) {
 Str = Buffer;
   }
 
-  return Result.convertFromString(Str, APFloat::rmNearestTiesToEven);
+  auto StatusOrErr =
+  Result.convertFromString(Str, APFloat::rmNearestTiesToEven);
+  assert(StatusOrErr && "Invalid floating point representation");
+  return StatusOrErr ? *StatusOrErr : APFloat::opInvalidOp;
 }
 
 static inline bool IsExponentPart(char c) {

diff  --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index afeed67e3f9e..e415054fc024 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -38,6 +38,7 @@ class StringRef;
 class APFloat;
 class raw_ostream;
 
+template  class Expected;
 template  class SmallVectorImpl;
 
 /// Enum that represents what fraction of the LSB truncated bits of an fp 
number
@@ -299,7 +300,7 @@ class IEEEFloat final : public APFloatBase {
   bool, roundingMode);
   opStatus convertFromZeroExtendedInteger(const integerPart *, unsigned int,
   bool, roundingMode);
-  opStatus convertFromString(StringRef, roundingMode);
+  Expected convertFromString(StringRef, roundingMode);
   APInt bitcastToAPInt() const;
   double convertToDouble() const;
   float convertToFloat() const;
@@ -525,8 +526,8 @@ class IEEEFloat final : public APFloatBase {
 bool *) const;
   opStatus convertFromUnsignedParts(const integerPart *, unsigned int,
 roundingMode);
-  opStatus convertFromHexadecimalString(StringRef, roundingMode);
-  opStatus convertFromDecimalString(StringRef, roundingMode);
+  Expected convertFromHexadecimalString(StringRef, roundingMode);
+  Expected convertFromDecimalString(StringRef, roundingMode);
   char *convertNormalToHexString(char *, unsigned int, bool,
  roundingMode) const;
   opStatus roundSignificandWithExponent(const integerPart *, unsigned int, int,
@@ -648,7 +649,7 @@ class DoubleAPFloat final : public APFloatBase {
   cmpResult compare(const DoubleAPFloat &RHS) const;
   bool bitwiseIsEqual(const DoubleAPFloat &RHS) const;
   APInt bitcastToAPInt() const;
-  opStatus convertFromString(StringRef, roundingMode);
+  Expected convertFromString(StringRef, roundingMode);
   opStatus next(bool nextDown);
 
   opStatus convertToInteger(MutableArrayRef Input,
@@ -1108,7 +1109,7 @@ class APFloat : public APFloatBase {
 APFLOAT_DISPATCH_ON_SEMANTICS(
 convertFromZeroExtendedInteger(Input, InputSize, IsSigned, RM));
   }
-  opStatus convertFromString(StringRef, roundingMode);
+  Expected convertFromString(StringRef, roundingMode);
   APInt bitcastToAPInt() const {
 APFLOAT_DISPATCH_ON_SEMANTICS(bitcastToAPInt());
   }

diff  --git a/llvm/lib/MC/MCParser/AsmParser.cpp 
b/llvm/lib/MC/MCParser/AsmParser.cpp
index 82318d081c9a..0c4eb953aa4e 100644
--- a/llvm/lib/MC/MCParser/AsmParser.cpp
+++ b/llvm/lib/MC/MCParser/AsmParser.cpp
@@ -3130,8 +3130,7 @@ bool AsmParser::parseRealValue(const fltSemantics 
&Semantics, APInt &Res) {
   Value = APFloat::getNaN(Semantics, false, ~0);
 else
   return TokError("invalid floating point literal");
-  } else if (Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven) ==
- APFloat::opInvalidOp)
+  } else if (!Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven))
 return TokError("invalid floating point literal");
   if (IsNeg)
 Value.changeSign();

diff  --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index c7500acaa729..f6999a6f236d 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -20,7 +20,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/Debug.h"
-#include "ll

[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

[Ehud Katz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc5fb73c5d1b3: [APFloat] Add recoverable string parsing 
errors to APFloat (authored by ekatz).

Changed prior to commit:
  https://reviews.llvm.org/D69770?vs=227917&id=236304#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69770

Files:
  clang/lib/Lex/LiteralSupport.cpp
  llvm/include/llvm/ADT/APFloat.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Support/APFloat.cpp
  llvm/lib/Support/StringRef.cpp
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
  llvm/unittests/ADT/APFloatTest.cpp

Index: llvm/unittests/ADT/APFloatTest.cpp
===
--- llvm/unittests/ADT/APFloatTest.cpp
+++ llvm/unittests/ADT/APFloatTest.cpp
@@ -10,8 +10,8 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
-#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -20,9 +20,17 @@
 
 using namespace llvm;
 
-static double convertToDoubleFromString(const char *Str) {
+static std::string convertToErrorFromString(StringRef Str) {
   llvm::APFloat F(0.0);
-  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  auto ErrOrStatus =
+  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  EXPECT_TRUE(!ErrOrStatus);
+  return toString(ErrOrStatus.takeError());
+}
+
+static double convertToDoubleFromString(StringRef Str) {
+  llvm::APFloat F(0.0);
+  EXPECT_FALSE(!F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven));
   return F.convertToDouble();
 }
 
@@ -1156,172 +1164,172 @@
   EXPECT_DEATH(APFloat(APFloat::IEEEsingle(), 0).convertToDouble(), "Float semantics are not IEEEdouble");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), 0).convertToFloat(),  "Float semantics are not IEEEsingle");
 }
+#endif
+#endif
 
-TEST(APFloatTest, StringDecimalDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ""), "Invalid string length");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+"), "String has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-"), "String has no digits");
+TEST(APFloatTest, StringDecimalError) {
+  EXPECT_EQ("Invalid string length", convertToErrorFromString(""));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("+"));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("-"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("\0", 1)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1\0", 2)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2", 3)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2e1", 5)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e\0", 3)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1\0", 4)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1" "\0" "2", 5)), "Invalid character in exponent");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("\0", 1)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1\0", 2)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2", 3)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2e1", 5)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e\0", 3)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1\0", 4)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1" "\0" "2", 5)));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0f"), "Invalid character in significand");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString("1.0f"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), ".."), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "..0"), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0.0"), "String contains multiple dots");
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString(".."));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("..0"));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("1.0.0"));
 }
 
-TEST(APFloatTest, StringDecimalSignificandDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "."), "Significand has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+."), "Significand has no digits");
-  EXPEC

[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/lib/Support/StringRef.cpp:593
+  if (!ErrOrStatus) {
+assert("Invalid floating point representation");
+return true;

This is an invalid assert.

lib/Support/StringRef.cpp:593:12: warning: implicit conversion turns string 
literal into bool: 'const char [38]' to 'bool' [-Wstring-conversion]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69770



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

[PATCH] D72247: Add Triple::isX86()

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61250 tests passed, 0 failed 
and 736 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72247



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

[PATCH] D72247: Add Triple::isX86()

[Kan Shengchen via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72247



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

[PATCH] D71387: pass -mabi to LTO linker only in RISC-V targets, enable RISC-V LTO

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen added a comment.

I'd like to discuss what is good way to pass target-abi in maillist 
 after I had 
a try to encoding ABI info into LLVM IR via module flag. 
any suggestions are welcome, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71387



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

[clang-tools-extra] f3f7dc3 - [APFloat] Fix compilation warnings

[Ehud Katz via cfe-commits]

Author: Ehud Katz
Date: 2020-01-06T11:30:40+02:00
New Revision: f3f7dc3d2990151a78b246a7a1485d0c13a9fb36

URL: 
https://github.com/llvm/llvm-project/commit/f3f7dc3d2990151a78b246a7a1485d0c13a9fb36
DIFF: 
https://github.com/llvm/llvm-project/commit/f3f7dc3d2990151a78b246a7a1485d0c13a9fb36.diff

LOG: [APFloat] Fix compilation warnings

Added: 


Modified: 
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
clang/lib/Lex/LiteralSupport.cpp
llvm/lib/Support/APFloat.cpp
llvm/lib/Support/StringRef.cpp
llvm/unittests/ADT/APFloatTest.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp 
b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
index 64806cee37ef..231e565f27e5 100644
--- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -86,11 +86,15 @@ MagicNumbersCheck::MagicNumbersCheck(StringRef Name, 
ClangTidyContext *Context)
 IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
 for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
   llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
-  FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+  if (!FloatValue.convertFromString(InputValue, DefaultRoundingMode)) {
+assert(false && "Invalid floating point representation");
+  }
   IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
 
   llvm::APFloat DoubleValue(llvm::APFloat::IEEEdouble());
-  DoubleValue.convertFromString(InputValue, DefaultRoundingMode);
+  if (!DoubleValue.convertFromString(InputValue, DefaultRoundingMode)) {
+assert(false && "Invalid floating point representation");
+  }
   IgnoredDoublePointValues.push_back(DoubleValue.convertToDouble());
 }
 llvm::sort(IgnoredFloatingPointValues.begin(),

diff  --git a/clang/lib/Lex/LiteralSupport.cpp 
b/clang/lib/Lex/LiteralSupport.cpp
index 66309183a144..5881852b1424 100644
--- a/clang/lib/Lex/LiteralSupport.cpp
+++ b/clang/lib/Lex/LiteralSupport.cpp
@@ -1053,8 +1053,11 @@ NumericLiteralParser::GetFloatValue(llvm::APFloat 
&Result) {
 
   auto StatusOrErr =
   Result.convertFromString(Str, APFloat::rmNearestTiesToEven);
-  assert(StatusOrErr && "Invalid floating point representation");
-  return StatusOrErr ? *StatusOrErr : APFloat::opInvalidOp;
+  if (!StatusOrErr) {
+assert(false && "Invalid floating point representation");
+return APFloat::opInvalidOp;
+  }
+  return *StatusOrErr;
 }
 
 static inline bool IsExponentPart(char c) {

diff  --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
index f6999a6f236d..d26c5e6cd2e6 100644
--- a/llvm/lib/Support/APFloat.cpp
+++ b/llvm/lib/Support/APFloat.cpp
@@ -4511,7 +4511,9 @@ hash_code hash_value(const APFloat &Arg) {
 APFloat::APFloat(const fltSemantics &Semantics, StringRef S)
 : APFloat(Semantics) {
   auto StatusOrErr = convertFromString(S, rmNearestTiesToEven);
-  assert(StatusOrErr && "Invalid floating point representation");
+  if (!StatusOrErr) {
+assert(false && "Invalid floating point representation");
+  }
 }
 
 APFloat::opStatus APFloat::convert(const fltSemantics &ToSemantics,

diff  --git a/llvm/lib/Support/StringRef.cpp b/llvm/lib/Support/StringRef.cpp
index 4142d130d519..b5db172cc1a3 100644
--- a/llvm/lib/Support/StringRef.cpp
+++ b/llvm/lib/Support/StringRef.cpp
@@ -590,7 +590,7 @@ bool StringRef::getAsDouble(double &Result, bool 
AllowInexact) const {
   APFloat F(0.0);
   auto ErrOrStatus = F.convertFromString(*this, APFloat::rmNearestTiesToEven);
   if (!ErrOrStatus) {
-assert("Invalid floating point representation");
+assert(false && "Invalid floating point representation");
 return true;
   }
 

diff  --git a/llvm/unittests/ADT/APFloatTest.cpp 
b/llvm/unittests/ADT/APFloatTest.cpp
index 927e1fe13671..db529a094c37 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -666,26 +666,12 @@ TEST(APFloatTest, Zero) {
 TEST(APFloatTest, DecimalStringsWithoutNullTerminators) {
   // Make sure that we can parse strings without null terminators.
   // rdar://14323230.
-  APFloat Val(APFloat::IEEEdouble());
-  Val.convertFromString(StringRef("0.00", 3),
-llvm::APFloat::rmNearestTiesToEven);
-  EXPECT_EQ(Val.convertToDouble(), 0.0);
-  Val.convertFromString(StringRef("0.01", 3),
-llvm::APFloat::rmNearestTiesToEven);
-  EXPECT_EQ(Val.convertToDouble(), 0.0);
-  Val.convertFromString(StringRef("0.09", 3),
-llvm::APFloat::rmNearestTiesToEven);
-  EXPECT_EQ(Val.convertToDouble(), 0.0);
-  Val.convertFromString(StringRef("0.095", 4),
-llvm::APFloat::rmNearestTiesToEven);
-  EXPECT_EQ(Val.convertToDouble(), 0.09);
-  Val.convertFromString(Stri

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

This change seems to be causing a problem with the nightly packages from 
apt.llvm.org.

  CMake Error at /usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake:1357 
(message):
  
The imported target "Bye" references the file
  
   "/usr/lib/llvm-10/lib/libBye.a"
  
but this file does not exist.  Possible reasons include:
  
* The file was deleted, renamed, or moved to another location.
  
* An install or uninstall procedure did not complete successfully.
  
* The installation package was faulty and contained
  
   "/usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake"
  
but not all the files it references.
  
  Call Stack (most recent call first):
  
/usr/lib/llvm-10/cmake/LLVMConfig.cmake:260 (include)
  
CMakeLists.txt:34 (find_package)
  
  -- Configuring incomplete, errors occurred!

For reference: we saw a similar issue with D69416 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D71365: expand printf when compiling HIP to AMDGPU

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds updated this revision to Diff 236312.
sameerds edited the summary of this revision.
sameerds added a comment.

Improved the test defined in clang/test/CodeGenHIP/printf.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71365

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGGPUBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/test/CodeGenHIP/printf-aggregate.cpp
  clang/test/CodeGenHIP/printf.cpp
  clang/test/Driver/hip-printf.hip
  llvm/include/llvm/Transforms/Utils/AMDGPUEmitPrintf.h
  llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt

Index: llvm/lib/Transforms/Utils/CMakeLists.txt
===
--- llvm/lib/Transforms/Utils/CMakeLists.txt
+++ llvm/lib/Transforms/Utils/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_llvm_component_library(LLVMTransformUtils
+  AMDGPUEmitPrintf.cpp
   ASanStackFrameLayout.cpp
   AddDiscriminators.cpp
   BasicBlockUtils.cpp
Index: llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp
@@ -0,0 +1,243 @@
+//===- AMDGPUEmitPrintf.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Utility function to lower a printf call into a series of device
+// library calls on the AMDGPU target.
+//
+// WARNING: This file knows about certain library functions. It recognizes them
+// by name, and hardwires knowledge of their semantics.
+//
+//===--===//
+
+#include "llvm/Transforms/Utils/AMDGPUEmitPrintf.h"
+#include "llvm/ADT/SparseBitVector.h"
+#include "llvm/Analysis/ValueTracking.h"
+#include "llvm/IR/IRBuilder.h"
+
+#include 
+
+using namespace llvm;
+
+#define DEBUG_TYPE "amdgpu-emit-printf"
+
+static bool isCString(const Value *Arg) {
+  auto Ty = Arg->getType();
+  auto PtrTy = dyn_cast(Ty);
+  if (!PtrTy)
+return false;
+
+  auto IntTy = dyn_cast(PtrTy->getElementType());
+  if (!IntTy)
+return false;
+
+  return IntTy->getBitWidth() == 8;
+}
+
+static Value *fitArgInto64Bits(IRBuilder<> &Builder, Value *Arg) {
+  auto Int64Ty = Builder.getInt64Ty();
+  auto Ty = Arg->getType();
+  if (auto IntTy = dyn_cast(Ty)) {
+switch (IntTy->getBitWidth()) {
+case 32:
+  return Builder.CreateZExt(Arg, Int64Ty);
+case 64:
+  return Arg;
+}
+  } else if (Ty->getTypeID() == Type::DoubleTyID) {
+return Builder.CreateBitCast(Arg, Int64Ty);
+  } else if (auto PtrTy = dyn_cast(Ty)) {
+return Builder.CreatePtrToInt(Arg, Int64Ty);
+  }
+
+  llvm_unreachable("unexpected type");
+  return Builder.getInt64(0);
+}
+
+static Value *callPrintfBegin(IRBuilder<> &Builder, Value *Version) {
+  auto Int64Ty = Builder.getInt64Ty();
+  auto M = Builder.GetInsertBlock()->getModule();
+  auto Fn = M->getOrInsertFunction("__ockl_printf_begin", Int64Ty, Int64Ty);
+  return Builder.CreateCall(Fn, Version);
+}
+
+static Value *callAppendArgs(IRBuilder<> &Builder, Value *Desc, int NumArgs,
+ Value *Arg0, Value *Arg1, Value *Arg2, Value *Arg3,
+ Value *Arg4, Value *Arg5, Value *Arg6,
+ bool IsLast) {
+  auto Int64Ty = Builder.getInt64Ty();
+  auto Int32Ty = Builder.getInt32Ty();
+  auto M = Builder.GetInsertBlock()->getModule();
+  auto Fn = M->getOrInsertFunction("__ockl_printf_append_args", Int64Ty,
+   Int64Ty, Int32Ty, Int64Ty, Int64Ty, Int64Ty,
+   Int64Ty, Int64Ty, Int64Ty, Int64Ty, Int32Ty);
+  auto IsLastValue = Builder.getInt32(IsLast);
+  auto NumArgsValue = Builder.getInt32(NumArgs);
+  return Builder.CreateCall(Fn, {Desc, NumArgsValue, Arg0, Arg1, Arg2, Arg3,
+ Arg4, Arg5, Arg6, IsLastValue});
+}
+
+static Value *appendArg(IRBuilder<> &Builder, Value *Desc, Value *Arg,
+bool IsLast) {
+  auto Arg0 = fitArgInto64Bits(Builder, Arg);
+  auto Zero = Builder.getInt64(0);
+  return callAppendArgs(Builder, Desc, 1, Arg0, Zero, Zero, Zero, Zero, Zero,
+Zero, IsLast);
+}
+
+// The device library does not provide strlen, so we build our own loop
+// here. While we are at it, we also include the terminating null in the length.
+static Value *getStrlenWithNull(IRBuilder<> &Builder, Value *Str) {
+  auto *Prev = Builder.GetInsertBlock();
+  Module *M = Prev->getModule();
+
+  auto CharZero = Builder.getInt8(0);
+  auto One = Builder.g

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal marked an inline comment as done.
vingeldal added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:198
cppcoreguidelines-avoid-magic-numbers (redirects to 
readability-magic-numbers) 
+   cppcoreguidelines-avoid-non-const-global-variables
cppcoreguidelines-c-copy-assignment-signature (redirects to 
misc-unconventional-assign-operator) 


vingeldal wrote:
> sylvestre.ledru wrote:
> > aaron.ballman wrote:
> > > vingeldal wrote:
> > > > sylvestre.ledru wrote:
> > > > > list.rst changed, you should update this!
> > > > > Thanks
> > > > > 
> > > > I'm a bit concerned with this previous change of of list.rst.
> > > > 
> > > > Now that I need to add a check to this file, I don't know what severity 
> > > > to give it. I can't find any definition of severity levels and I really 
> > > > want to avoid getting into a long discussion, or having different 
> > > > reviewers not agreeing on my patch, concerning what severity we should 
> > > > give this check.
> > > > Is there any way I can find some kind of guideline on how to set the 
> > > > severity to avoid an opinionated discussion about severity level?
> > > I'd like to follow that discussion so that I can be consistent with my 
> > > review advice, too. 
> > Good point!
> > 
> > I propose this patch to explain
> > https://reviews.llvm.org/D71963
> > 
> > this is a lazy copy and paste of 
> > https://github.com/Ericsson/codechecker/blob/master/config/config.md
> > 
> Great, will make sure to contribute some feedback to that.
It seems to me like this list is still generated the old way if one uses 
add_new_check.py to add a new check.
@sylvestre.ledru could you please also adapt that script to generate the table 
instead of the list?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70265



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#1805108 , @0x8000- wrote:

> In D54943#1804943 , @JonasToth wrote:
>
> > - Merge branch 'master' into feature_transform_const.patch
> > - link clangAnalysis to the cppcoreguidelines-module
> > - fix InitListExpr as well
>
>
> This last version too builds with RelWithDebInfo and fails with Debug 
> configuration (both clean builds).


So it only fails in debug-build?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[clang-tools-extra] 7180d95 - Fix MSVC "not all control paths return a value" warning. NFCI.

[Simon Pilgrim via cfe-commits]

Author: Simon Pilgrim
Date: 2020-01-06T10:20:20Z
New Revision: 7180d9568df7d9198a75cfb5a156d9e60329794a

URL: 
https://github.com/llvm/llvm-project/commit/7180d9568df7d9198a75cfb5a156d9e60329794a
DIFF: 
https://github.com/llvm/llvm-project/commit/7180d9568df7d9198a75cfb5a156d9e60329794a.diff

LOG: Fix MSVC "not all control paths return a value" warning. NFCI.

Added: 


Modified: 
clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp 
b/clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
index a9c95e9f3810..dd7c24d0c7ef 100644
--- a/clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
@@ -95,6 +95,7 @@ static Optional changeValue(const VarDecl &Var,
   return fixIfNotDangerous(*IgnoredParens, buildQualifier(Qualifier));
 return None;
   }
+  llvm_unreachable("Unknown QualifierPolicy enum");
 }
 
 static Optional changePointerItself(const VarDecl &Var,



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

[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:8043
+
+  Error VisitFunctionTypeLoc(FunctionTypeLoc From) {
+auto To = ToL.castAs();

martong wrote:
> a_sidorin wrote:
> > Does this import interacts well with type loc import partially done at 
> > L3258 (VisitFunctionDecl)? Should we merge them?
> I think we should check here whether the given ParmVarDecl had been imported 
> previously and if yes then set that with `setParam`, otherwise it could be 
> set to a nullptr. @balazske what do you think?
Probably we could get the `FunctionTypeLoc` from `TInfo` (this is the first 
TypeLoc there?) and set the parameters (that are set to nullptr at line 8055), 
at this loop in `VisitFunctionDecl`:
```
  // Set the parameters.
  for (auto *Param : Parameters) {
Param->setOwningFunction(ToFunction);
ToFunction->addDeclInternal(Param);
  }
  ToFunction->setParams(Parameters);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71018



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

[PATCH] D72227: Add options for clang to align branches within 32B boundary

[Kan Shengchen via Phabricator via cfe-commits]

skan updated this revision to Diff 236329.
skan marked 5 inline comments as done.

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

https://reviews.llvm.org/D72227

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/intel-align-branch.c

Index: clang/test/Driver/intel-align-branch.c
===
--- /dev/null
+++ clang/test/Driver/intel-align-branch.c
@@ -0,0 +1,32 @@
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-boundary=32 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDARY
+// CHECK-BOUNDARY: "-mllvm" "-x86-align-branch-boundary=32"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=jcc -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-JCC
+// CHECK-JCC: "-mllvm" "-x86-align-branch=jcc"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=fused -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-FUSED
+// CHECK-FUSED: "-mllvm" "-x86-align-branch=fused"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=jmp -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-JMP
+// CHECK-JMP: "-mllvm" "-x86-align-branch=jmp"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=call -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-CALL
+// CHECK-CALL: "-mllvm" "-x86-align-branch=call"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=ret -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-RET
+// CHECK-RET: "-mllvm" "-x86-align-branch=ret"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=indirect -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-INDIRECT
+// CHECK-INDIRECT: "-mllvm" "-x86-align-branch=indirect"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=fused+jcc+jmp+ret+call+indirect -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-BRANCH
+// CHECK-BRANCH: "-mllvm" "-x86-align-branch=fused+jcc+jmp+ret+call+indirect"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-prefix-size=4 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-PREFIX
+// CHECK-PREFIX: "-mllvm" "-x86-align-branch-prefix-size=4"
+//
+// RUN: %clang -target x86_64-unknown-unknown -mno-branches-within-32B-boundaries -mbranches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-TOTAL1
+// CHECK-TOTAL1: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"
+//
+// RUN: %clang -target x86_64-unknown-unknown -mbranches-within-32B-boundaries -mno-branches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-TOTAL2
+// CHECK-TOTAL2-NOT: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2007,8 +2007,55 @@
 CmdArgs.push_back("-mpacked-stack");
 }
 
+static void alignBranchesOptions(const Driver &D, const ArgList &Args,
+ ArgStringList &CmdArgs) {
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+StringRef Value = A->getValue();
+uint64_t Num;
+if (!Value.getAsInteger(10, Num) && Num >= 32 && llvm::isPowerOf2_64(Num)) {
+  CmdArgs.push_back("-mllvm");
+  CmdArgs.push_back(
+  Args.MakeArgString("-x86-align-branch-boundary=" + Value));
+} else {
+  D.Diag(diag::err_drv_unsupported_option_argument)
+  << A->getOption().getName() << Value;
+}
+  }
+
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_EQ)) {
+StringRef Value = A->getValue();
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=" + Value));
+  }
+
+  if (const Arg *A =
+  Args.getLastArg(options::OPT_malign_branch_prefix_size_EQ)) {
+StringRef Value = A->getValue();
+uint8_t Num;
+if (!Value.getAsInteger(10, Num) && Num <= 4) {
+  CmdArgs.push_back("-mllvm");
+  CmdArgs.push_back(
+  Args.MakeArgString("-x86-align-branch-prefix-size=" + Value));
+} else {
+  D.Diag(diag::err_drv_unsupported_option_argument)
+  << A->getOption().getName() << Value;
+}
+  }
+
+  if (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries,
+   options::OPT_mno_branches_within_32B_boundaries, false)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-boundary=32"));
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=fused+jcc+jmp"));
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=4"));
+  }
+}
+
 void Clang::AddX86TargetArgs(const ArgList &Args,

[PATCH] D72227: Add options for clang to align branches within 32B boundary

[Kan Shengchen via Phabricator via cfe-commits]

skan added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2012
+ ArgStringList &CmdArgs) {
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+StringRef Value = A->getValue();

MaskRay wrote:
> if (!TC.getTriple().isX86()) {
>   D.Diag(diag::err_drv_unsupported_opt_for_target)
>   << A->getAsString(Args) << TripleStr;
> return;
>   }
> 
> I will add Triple::isX86 in D72247.
Is this check necessary?  `alignBranchesOptions` is only called by 
`Clang::AddX86TargetArgs` and `ClangAs::AddX86TargetArgs`, so the triple is 
always X86. 

If `-target aarch64` is passed, `-mbranches-within-32B-boundaries` will not be 
consumed and user can get an warning. So is `-malign-branch` etc.

> clang-10: warning: argument unused during compilation: 
> '-mbranches-within-32B-boundaries' [-Wunused-command-line-argument]





Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2045
+
+  if (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries,
+   options::OPT_mno_branches_within_32B_boundaries, false)) {

MaskRay wrote:
> `OPT_mbranches_within_32B_boundaries` should provide default values which can 
> be overridden by more specific options.
Currently, `-mbranches-within-32B-boundaries` is equivalent to 
`-malign-branch-boundary=32 -malign-branch=fused+jcc+jmp 
-malign-branch-prefix-size=4

What is expected behaviour would be very confusing if specific options could 
override `-mbranches-within-32B-boundaries`. For example, if passed options are

```
-mbranches-within-32B-boundaries -malign-branch-boundary=32 
-mno-branches-within-32B-boundaries
```
What should the value of `-malign-branch-boundary` be?  Is it 32 or 0?

If we think `-mno-branches-within-32B-boundaries` is the negative form of 
`-mbranches-within-32B-boundaries` , then `-malign-branch-boundary` should be 
32.

Or if we think `-mno-branches-within-32B-boundaries` wins since it appears at 
the end, and `-mno-branches-within-32B-boundaries` means no need to align 
branches, `-malign-branch-boundary` should be 0.

As long as we don't support specific options could override 
`-mbranches-within-32B-boundaries`, the trouble disappears  :-)





Comment at: clang/test/Driver/intel-align-branch.c:28
+//
+// // RUN: %clang -target x86_64-unknown-unknown 
-mbranches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s 
--check-prefix=CHECK-TOTAL
+// CHECK-TOTAL: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" 
"-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"

MaskRay wrote:
> Since you have a `-mno.. -m..` test, the `-m..` test is redundant.
Done



Comment at: clang/test/Driver/intel-align-branch.c:35
+// RUN: %clang -target x86_64-unknown-unknown -mbranches-within-32B-boundaries 
-mno-branches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s 
--check-prefix=CHECK-TOTAL3
+// CHECK-TOTAL3-NOT: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" 
"-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"

MaskRay wrote:
> Add a `-target aarch64` test that other targets will get an error.
> 
> (I am lazy and usually just write `-target x86_64` (generic ELF) and omit 
> `-unknown-known`)
Currently, if  `-target aarch64` is passed to clang, we will get a warning 

> clang-10: warning: argument unused during compilation: 
> '-mbranches-within-32B-boundaries' [-Wunused-command-line-argument]

Is this prompt not clear enough?



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

https://reviews.llvm.org/D72227



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

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D61446#1805413 , @svenvh wrote:

> This change seems to be causing a problem with the nightly packages from 
> apt.llvm.org.


https://reviews.llvm.org/D72255 should do the trick (?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446



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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 236339.
JonasToth added a comment.

- fix false positive with parenListExpr, dont match so narrow for them
- try removing more false positives, does not work


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -62,13 +62,18 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
+  std::string buffer;
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
+if (!By)
+  break;
+
 llvm::raw_string_ostream stream(buffer);
 By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+Chain.emplace_back(StringRef(stream.str()).trim().str());
 E = dyn_cast(By);
+buffer.clear();
   }
   return Chain;
 }
@@ -882,38 +887,36 @@
 }
 
 TEST(ExprMutationAnalyzerTest, CommaExprWithAnAssigment) {
-  const auto AST =
-  buildASTFromCodeWithArgs("void f() { int x; int y; (x, y) = 5; }",
-   {"-Wno-unused-value"});
+  const auto AST = buildASTFromCodeWithArgs(
+  "void f() { int x; int y; (x, y) = 5; }", {"-Wno-unused-value"});
   const auto Results =
   match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
   EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, CommaExprWithDecOp) {
-  const auto AST =
-  buildASTFromCodeWithArgs("void f() { int x; int y; (x, y)++; }",
-   {"-Wno-unused-value"});
+  const auto AST = buildASTFromCodeWithArgs(
+  "void f() { int x; int y; (x, y)++; }", {"-Wno-unused-value"});
   const auto Results =
   match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
   EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, CommaExprWithNonConstMemberCall) {
-  const auto AST =
-  buildASTFromCodeWithArgs("class A { public: int mem; void f() { mem ++; } };"
-   "void fn() { A o1, o2; (o1, o2).f(); }",
-   {"-Wno-unused-value"});
+  const auto AST = buildASTFromCodeWithArgs(
+  "class A { public: int mem; void f() { mem ++; } };"
+  "void fn() { A o1, o2; (o1, o2).f(); }",
+  {"-Wno-unused-value"});
   const auto Results =
   match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext());
   EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, CommaExprWithConstMemberCall) {
-  const auto AST =
-  buildASTFromCodeWithArgs("class A { public: int mem; void f() const  { } };"
-   "void fn() { A o1, o2; (o1, o2).f(); }",
-   {"-Wno-unused-value"});
+  const auto AST = buildASTFromCodeWithArgs(
+  "class A { public: int mem; void f() const  { } };"
+  "void fn() { A o1, o2; (o1, o2).f(); }",
+  {"-Wno-unused-value"});
   const auto Results =
   match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
@@ -957,11 +960,10 @@
 }
 
 TEST(ExprMutationAnalyzerTest, CommaExprWithAmpersandOp) {
-  const auto AST =
-  buildASTFromCodeWithArgs("class A { public: int mem;};"
-   "void fn () { 

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm concerned about just making this fallthrough to 'false'.  These ARE 
integral promotions, we just don't know the type size. What happens if you 
instantiate this template?  Will it still give the correct 
answer/type/diagnosis?

In the case of the crash, I would suspect that the expression 'if (c)' should 
just be dependent and skipped from the semantic analysis because of it.


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

https://reviews.llvm.org/D72242



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

[clang] d45aafa - [clang-format] fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Mitchell Balan via cfe-commits]

Author: Mitchell Balan
Date: 2020-01-06T09:21:41-05:00
New Revision: d45aafa2fbcf66f3dafdc7c5e0a0ce3709914cbc

URL: 
https://github.com/llvm/llvm-project/commit/d45aafa2fbcf66f3dafdc7c5e0a0ce3709914cbc
DIFF: 
https://github.com/llvm/llvm-project/commit/d45aafa2fbcf66f3dafdc7c5e0a0ce3709914cbc.diff

LOG: [clang-format] fix conflict between FormatStyle::BWACS_MultiLine and 
BeforeCatch/BeforeElse

Summary:
Found a bug introduced with BraceWrappingFlags AfterControlStatement MultiLine. 
This feature conflicts with the existing BeforeCatch and BeforeElse flags.

For example, our team uses BeforeElse.

if (foo ||
bar) {
  doSomething();
}
else {
  doSomethingElse();
}

If we enable MultiLine (which we'd really love to do) we expect it to work like 
this:

if (foo ||
bar)
{
  doSomething();
}
else {
  doSomethingElse();
}

What we actually get is:

if (foo ||
bar)
{
  doSomething();
}
else
{
  doSomethingElse();
}

Reviewers: MyDeveloperDay, Bouska, mitchell-stellar

Patch by: pastey

Subscribers: Bouska, cfe-commits

Tags: clang

Differential Revision: https://reviews.llvm.org/D71939

Added: 


Modified: 
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/unittests/Format/FormatTest.cpp

Removed: 




diff  --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 8b8d357d9cbe..fec85f1174da 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -326,6 +326,19 @@ class LineJoiner {
   FormatStyle::BWACS_Always)
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
+} else if (I[1]->First->is(tok::l_brace) &&
+   TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
+   Style.BraceWrapping.AfterControlStatement ==
+   FormatStyle::BWACS_MultiLine) {
+  // This case if 
diff erent from the upper BWACS_MultiLine processing
+  // in that a preceding r_brace is not on the same line as else/catch
+  // most likely because of BeforeElse/BeforeCatch set to true.
+  // If the line length doesn't fit ColumnLimit, leave l_brace on the
+  // next line to respect the BWACS_MultiLine.
+  return (Style.ColumnLimit == 0 ||
+  TheLine->Last->TotalLength <= Style.ColumnLimit)
+ ? 1
+ : 0;
 }
 // Try to merge either empty or one-line block if is precedeed by control
 // statement token

diff  --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 069542683c0d..2d67b9759d7f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@ TEST_F(FormatTest, MultiLineControlStatements) {
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column 
limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception &bar) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column 
limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 
//===--===//
@@ -14881,7 +14916,7 @@ TEST_F(FormatTest, AmbersandInLamda) {
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);



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

[PATCH] D71939: clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd45aafa2fbcf: [clang-format] fix conflict between 
FormatStyle::BWACS_MultiLine and… (authored by mitchell-stellar).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71939

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column 
limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception &bar) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column 
limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 
//===--===//
@@ -14881,7 +14916,7 @@
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
- TEST_F(FormatTest, SpacesInConditionalStatement) {
+TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -326,6 +326,19 @@
   FormatStyle::BWACS_Always)
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
+} else if (I[1]->First->is(tok::l_brace) &&
+   TheLine->First->isOneOf(tok::kw_else, tok::kw_catch) &&
+   Style.BraceWrapping.AfterControlStatement ==
+   FormatStyle::BWACS_MultiLine) {
+  // This case if different from the upper BWACS_MultiLine processing
+  // in that a preceding r_brace is not on the same line as else/catch
+  // most likely because of BeforeElse/BeforeCatch set to true.
+  // If the line length doesn't fit ColumnLimit, leave l_brace on the
+  // next line to respect the BWACS_MultiLine.
+  return (Style.ColumnLimit == 0 ||
+  TheLine->Last->TotalLength <= Style.ColumnLimit)
+ ? 1
+ : 0;
 }
 // Try to merge either empty or one-line block if is precedeed by control
 // statement token


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1565,6 +1565,41 @@
 "  baz();\n"
 "}",
 format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  40; // to concentrate at brace wrapping, not line wrap due to column limit
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"} catch (Exception &bar) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(Exception&bar){baz();}", Style));
+  Style.ColumnLimit =
+  20; // to concentrate at brace wrapping, not line wrap due to column limit
+
+  Style.BraceWrapping.BeforeElse = true;
+  EXPECT_EQ(
+  "if (foo) {\n"
+  "  bar();\n"
+  "}\n"
+  "else if (baz ||\n"
+  " quux)\n"
+  "{\n"
+  "  foobar();\n"
+  "}\n"
+  "else {\n"
+  "  barbaz();\n"
+  "}",
+  format("if(foo){bar();}else if(baz||quux){foobar();}else{barbaz();}",
+ Style));
+
+  Style.BraceWrapping.BeforeCatch = true;
+  EXPECT_EQ("try {\n"
+"  foo();\n"
+"}\n"
+"catch (...) {\n"
+"  baz();\n"
+"}",
+format("try{foo();}catch(...){baz();}", Style));
 }
 
 //===--===//
@@ -14881,7 +14916,7 @@
   verifyFormat("auto lambda = [&a = a]() { a = 2; };"

[PATCH] D71510: [clang][checkers] Added new checker 'error-return-checker'. (WIP)

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 236349.
balazske added a comment.

- Added variadic functions, improved comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71510

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/error-return.c

Index: clang/test/Analysis/error-return.c
===
--- /dev/null
+++ clang/test/Analysis/error-return.c
@@ -0,0 +1,584 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.ErrorReturn -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+/*
+Functions from CERT ERR33-C that should be checked for error:
+
+void *aligned_alloc( size_t alignment, size_t size );
+errno_t asctime_s(char *buf, rsize_t bufsz, const struct tm *time_ptr);
+int at_quick_exit( void (*func)(void) );
+int atexit( void (*func)(void) );
+void* bsearch( const void *key, const void *ptr, size_t count, size_t size,
+   int (*comp)(const void*, const void*) );
+void* bsearch_s( const void *key, const void *ptr, rsize_t count, rsize_t size,
+ int (*comp)(const void *, const void *, void *),
+ void *context );
+wint_t btowc( int c );
+size_t c16rtomb( char * restrict s, char16_t c16, mbstate_t * restrict ps );
+size_t c32rtomb( char * restrict s, char32_t c32, mbstate_t * restrict ps );
+void* calloc( size_t num, size_t size );
+clock_t clock(void);
+int cnd_broadcast( cnd_t *cond );
+int cnd_init( cnd_t* cond );
+int cnd_signal( cnd_t *cond );
+int cnd_timedwait( cnd_t* restrict cond, mtx_t* restrict mutex,
+   const struct timespec* restrict time_point );
+int cnd_wait( cnd_t* cond, mtx_t* mutex );
+errno_t ctime_s(char *buffer, rsize_t bufsz, const time_t *time);
+int fclose( FILE *stream );
+int fflush( FILE *stream );
+int fgetc( FILE *stream );
+int fgetpos( FILE *restrict stream, fpos_t *restrict pos );
+char *fgets( char *restrict str, int count, FILE *restrict stream );
+wint_t fgetwc( FILE *stream );
+FILE *fopen( const char *restrict filename, const char *restrict mode );
+errno_t fopen_s(FILE *restrict *restrict streamptr,
+const char *restrict filename,
+const char *restrict mode);
+int fprintf( FILE *restrict stream, const char *restrict format, ... );
+int fprintf_s(FILE *restrict stream, const char *restrict format, ...);
+int fputc( int ch, FILE *stream );
+int fputs( const char *restrict str, FILE *restrict stream );
+wint_t fputwc( wchar_t ch, FILE *stream );
+int fputws( const wchar_t * restrict str, FILE * restrict stream );
+size_t fread( void *restrict buffer, size_t size, size_t count,
+  FILE *restrict stream );
+FILE *freopen( const char *restrict filename, const char *restrict mode,
+   FILE *restrict stream );
+errno_t freopen_s(FILE *restrict *restrict newstreamptr,
+  const char *restrict filename, const char *restrict mode,
+  FILE *restrict stream);
+int fscanf( FILE *restrict stream, const char *restrict format, ... );
+int fscanf_s(FILE *restrict stream, const char *restrict format, ...);
+int fseek( FILE *stream, long offset, int origin );
+int fsetpos( FILE *stream, const fpos_t *pos );
+long ftell( FILE *stream );
+int fwprintf( FILE *restrict stream,
+  const wchar_t *restrict format, ... );
+int fwprintf_s( FILE *restrict stream,
+const wchar_t *restrict format, ...);
+size_t fwrite( const void *restrict buffer, size_t size, size_t count,
+   FILE *restrict stream ); // more exact error return: < count
+int fwscanf( FILE *restrict stream,
+ const wchar_t *restrict format, ... );
+int fwscanf_s( FILE *restrict stream,
+   const wchar_t *restrict format, ...);
+int getc( FILE *stream );
+int getchar(void);
+char *getenv( const char *name );
+errno_t getenv_s( size_t *restrict len, char *restrict value,
+  rsize_t valuesz, const char *restrict name );
+char *gets_s( char *str, rsize_t n );
+wint_t getwc( FILE *stream );
+wint_t getwchar(void);
+struct tm *gmtime( const time_t *time );
+struct tm *gmtime_s(const time_t *restrict time, struct tm *restrict result);
+struct tm *localtime( const time_t *time );
+struct tm *localtime_s(const time_t *restrict time, struct tm *restrict result);
+void* malloc( size_t size );
+int mblen( const char* s, size_t n );
+size_t mbrlen( const char *restrict s, size_t n, mbstate_t *restrict ps );
+size_t mbrtoc16( char16_t * restrict pc16, const char * restrict s,
+ size_t n, mbstate_t * restrict ps );
+size_t mbrtoc32( char32_t restrict * pc32, const char * restrict s,
+ size_t n, mbstate_t * r

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:77
   "invalid unwind library name in argument '%0'">;
+def err_drv_incompatible_options : Error<"'%0' and '%1' cannot be used 
together">;
 def err_drv_incompatible_unwindlib : Error<

We already have `err_drv_argument_not_allowed_with`, which can be used instead 
of adding this.



Comment at: clang/include/clang/Driver/Options.td:1703
   HelpText<"Recognize and construct Pascal-style string literals">;
+def fpatchable_function_entry : Joined<["-"], "fpatchable-function-entry=">, 
Group, Flags<[CC1Option]>,
+  HelpText<"Generate N NOPs at function entry">;

Names of options with an equals sign end in `_EQ`.



Comment at: clang/test/Driver/fpatchable-function-entry.c:11
+// RUN: not %clang -target i386 -fsyntax-only %s 
-fpatchable-function-entry=1,1 2>&1 | FileCheck --check-prefix=NONZERO %s
+// NONZERO: error: the clang compiler does not support '1'
+

I think we should have a more descriptive message here, maybe something like:

  error: the second parameter of '-fpatchable-function-entry=' must be '0' or 
omitted


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Matt Arsenault via Phabricator via cfe-commits]

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

In D69878#1801508 , @cameron.mcinally 
wrote:

> This is looking pretty good to me, but I'm ignoring some of the target 
> specific code that I'm not familiar with.
>
> Is `denormal-fp-math` influenced by `-Ofast`? Or are there plans for that? 
> Seems like `-Ofast` should imply DAZ and FTZ (if supported by target).


Yes, through the toolchain handling. I copied the logic for when crtfastmath is 
linked for the default mode for x86.

> I think we discussed this before, but it's worth repeating. If 
> `denormal-fp-math` isn't specified, we default to IEEE behavior, right? When 
> this lands in master, there could be an unexpected performance hit for 
> targets that aren't paying attention. E.g. I want to use `denormal-fp-math` 
> to toggle whether a FSUB(-0.0,X) is converted to a FNEG(X) in 
> SelectionDAGBuilder.
> 
> Apologies in advance if this has been discussed recently. I've been 
> distracted with another project for the passed few months...

Yes, ieee should be the default. The dependent patches start adding the 
attribute by default for platforms with flushing enabled with fast math




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2311
+  bool TrappingMath = true;
 // overriden by ffp-exception-behavior?
   bool RoundingFPMath = false;

cameron.mcinally wrote:
> Last line of comment was not removed.
> 
> Also, is it safe to remove `TrappingMathPresent`? Is that part of the 
> work-in-progress to support `ffp-exception-behavior`?
I think this is a rebase gone bad. The patch changing the strict math was 
revered and recommitted and I probably broke this


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

https://reviews.llvm.org/D69878



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

[PATCH] D72268: [ARM,MVE] Support -ve offsets in gather-load intrinsics.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: dmgreen, miyuki, MarkMurrayARM.
Herald added subscribers: llvm-commits, cfe-commits, kristof.beyls.
Herald added projects: clang, LLVM.

The ACLE intrinsics with `gather_base` or `scatter_base` in the name
are wrappers on the MVE load/store instructions that take a vector of
base addresses and an immediate offset. The immediate offset can be up
to 127 times the alignment unit, and it can be positive or negative.

At the MC layer, we got that right. But in the Sema error checking for
the wrapping intrinsics, the offset was erroneously constrained to be
positive.

To fix this I've adjusted the `imm_mem7bit` class in the Tablegen that
defines the intrinsics. But that causes integer literals like
`0xfe04` to appear in the autogenerated calls to
`SemaBuiltinConstantArgRange`, which provokes a compiler warning
because that's out of the non-overflowing range of an `int64_t`. So
I've also tweaked `MveEmitter` to emit that as `-0x1fc` instead.

Updated the tests of the Sema checks themselves, and also adjusted a
random sample of the CodeGen tests to actually use negative offsets
and prove they get all the way through code generation without causing
a crash.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72268

Files:
  clang/include/clang/Basic/arm_mve_defs.td
  clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
  clang/test/Sema/arm-mve-immediates.c
  clang/utils/TableGen/MveEmitter.cpp
  llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll

Index: llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
===
--- llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
+++ llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
@@ -191,11 +191,11 @@
 define arm_aapcs_vfpcc <2 x i64> @test_vldrdq_gather_base_u64(<2 x i64> %addr) {
 ; CHECK-LABEL: test_vldrdq_gather_base_u64:
 ; CHECK:   @ %bb.0: @ %entry
-; CHECK-NEXT:vldrd.u64 q1, [q0, #336]
+; CHECK-NEXT:vldrd.u64 q1, [q0, #-336]
 ; CHECK-NEXT:vmov q0, q1
 ; CHECK-NEXT:bx lr
 entry:
-  %0 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> %addr, i32 336)
+  %0 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> %addr, i32 -336)
   ret <2 x i64> %0
 }
 
@@ -221,12 +221,12 @@
 ; CHECK-LABEL: test_vldrdq_gather_base_wb_u64:
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
-; CHECK-NEXT:vldrd.u64 q1, [q0, #328]!
+; CHECK-NEXT:vldrd.u64 q1, [q0, #-328]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <2 x i64>, <2 x i64>* %addr, align 8
-  %1 = call { <2 x i64>, <2 x i64> } @llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> %0, i32 328)
+  %1 = call { <2 x i64>, <2 x i64> } @llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> %0, i32 -328)
   %2 = extractvalue { <2 x i64>, <2 x i64> } %1, 1
   store <2 x i64> %2, <2 x i64>* %addr, align 8
   %3 = extractvalue { <2 x i64>, <2 x i64> } %1, 0
@@ -297,13 +297,13 @@
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vmsr p0, r0
 ; CHECK-NEXT:vpst
-; CHECK-NEXT:vldrdt.u64 q1, [q0, #1000]
+; CHECK-NEXT:vldrdt.u64 q1, [q0, #-1000]
 ; CHECK-NEXT:vmov q0, q1
 ; CHECK-NEXT:bx lr
 entry:
   %0 = zext i16 %p to i32
   %1 = call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %0)
-  %2 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.predicated.v2i64.v2i64.v4i1(<2 x i64> %addr, i32 1000, <4 x i1> %1)
+  %2 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.predicated.v2i64.v2i64.v4i1(<2 x i64> %addr, i32 -1000, <4 x i1> %1)
   ret <2 x i64> %2
 }
 
@@ -728,12 +728,12 @@
 ; CHECK-LABEL: test_vldrwq_gather_base_wb_f32:
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
-; CHECK-NEXT:vldrw.u32 q1, [q0, #64]!
+; CHECK-NEXT:vldrw.u32 q1, [q0, #-64]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <4 x i32>, <4 x i32>* %addr, align 8
-  %1 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.v4f32.v4i32(<4 x i32> %0, i32 64)
+  %1 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.v4f32.v4i32(<4 x i32> %0, i32 -64)
   %2 = extractvalue { <4 x float>, <4 x i32> } %1, 1
   store <4 x i32> %2, <4 x i32>* %addr, align 8
   %3 = extractvalue { <4 x float>, <4 x i32> } %1, 0
@@ -782,14 +782,14 @@
 ; CHECK-NEXT:vmsr p0, r1
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
 ; CHECK-NEXT:vpst
-; CHECK-NEXT:vldrwt.u32 q1, [q0, #352]!
+; CHECK-NEXT:vldrwt.u32 q1, [q0, #-352]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <4 x i32>, <4 x i32>* %addr, align 8
   %1 = zext i16 %p to i32
   %2 = call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %1)
-  %3 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.predicated.v4f32.v4i32.v4i1(<4 x i32> %0, i32 352, <4 x i1> %2)
+  %3 = call { <4 x float>, <4 x i32> 

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: dmgreen, rjmccall.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.

A few of the ARM MVE builtins directly return a structure type. This
causes an assertion failure at code-gen time if you try to assign the
result of the builtin to a variable, because the `RValue` created in
`EmitBuiltinExpr` from the `llvm::Value` produced by codegen is always
made by `RValue::get()`, which creates a non-aggregate `RValue` that
will fail an assertion when `AggExprEmitter::withReturnValueSlot`
calls `Src.getAggregatePointer()`.

To fix it, I've added a check in `EmitBuiltinExpr` so that it will
construct an aggregate `RValue` if the provided return value slot
contains a valid `Address` whose type is a pointer to struct. As far
as I can see that makes no difference to any existing builtin, and it
fixes the crash in the structure-typed MVE ones.

The existing code-gen tests for those MVE builtins pass the returned
structure type directly to the C `return` statement, which apparently
managed to avoid that particular code path, so we didn't notice the
crash. But as soon as you try to assign the returned structure into
(for example) a local variable, it does crash. Added an extra test
that reproduced the bug before this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72271

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/arm-mve-intrinsics/vld24.c


Index: clang/test/CodeGen/arm-mve-intrinsics/vld24.c
===
--- clang/test/CodeGen/arm-mve-intrinsics/vld24.c
+++ clang/test/CodeGen/arm-mve-intrinsics/vld24.c
@@ -98,3 +98,27 @@
 vst2q_f16(addr, value);
 #endif /* POLYMORPHIC */
 }
+
+// CHECK-LABEL: @load_via_variable(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call { <8 x i16>, <8 x i16> } 
@llvm.arm.mve.vld2q.v8i16.p0i16(i16* [[ADDR:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = extractvalue { <8 x i16>, <8 x i16> } 
[[TMP0]], 0
+// CHECK-NEXT:[[TMP2:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T:%.*]] 
undef, <8 x i16> [[TMP1]], 0, 0
+// CHECK-NEXT:[[TMP3:%.*]] = extractvalue { <8 x i16>, <8 x i16> } 
[[TMP0]], 1
+// CHECK-NEXT:[[TMP4:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T]] [[TMP2]], 
<8 x i16> [[TMP3]], 0, 1
+// CHECK-NEXT:store <8 x i16> [[TMP1]], <8 x i16>* [[VALUES:%.*]], align 8
+// CHECK-NEXT:[[ARRAYIDX4:%.*]] = getelementptr inbounds <8 x i16>, <8 x 
i16>* [[VALUES]], i32 1
+// CHECK-NEXT:store <8 x i16> [[TMP3]], <8 x i16>* [[ARRAYIDX4]], align 8
+// CHECK-NEXT:ret void
+//
+void load_via_variable(const uint16_t *addr, uint16x8_t *values)
+{
+uint16x8x2_t v;
+#ifdef POLYMORPHIC
+v = vld2q(addr);
+#else /* POLYMORPHIC */
+v = vld2q_u16(addr);
+#endif /* POLYMORPHIC */
+values[0] = v.val[0];
+values[1] = v.val[1];
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -4328,8 +4328,13 @@
   }
 
   // See if we have a target specific builtin that needs to be lowered.
-  if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E, ReturnValue))
+  if (Value *V = EmitTargetBuiltinExpr(BuiltinID, E, ReturnValue)) {
+if (!ReturnValue.isNull() &&
+
ReturnValue.getValue().getType()->getPointerElementType()->isStructTy())
+  return RValue::getAggregate(ReturnValue.getValue(),
+  ReturnValue.isVolatile());
 return RValue::get(V);
+  }
 
   ErrorUnsupported(E, "builtin function");
 


Index: clang/test/CodeGen/arm-mve-intrinsics/vld24.c
===
--- clang/test/CodeGen/arm-mve-intrinsics/vld24.c
+++ clang/test/CodeGen/arm-mve-intrinsics/vld24.c
@@ -98,3 +98,27 @@
 vst2q_f16(addr, value);
 #endif /* POLYMORPHIC */
 }
+
+// CHECK-LABEL: @load_via_variable(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[TMP0:%.*]] = call { <8 x i16>, <8 x i16> } @llvm.arm.mve.vld2q.v8i16.p0i16(i16* [[ADDR:%.*]])
+// CHECK-NEXT:[[TMP1:%.*]] = extractvalue { <8 x i16>, <8 x i16> } [[TMP0]], 0
+// CHECK-NEXT:[[TMP2:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T:%.*]] undef, <8 x i16> [[TMP1]], 0, 0
+// CHECK-NEXT:[[TMP3:%.*]] = extractvalue { <8 x i16>, <8 x i16> } [[TMP0]], 1
+// CHECK-NEXT:[[TMP4:%.*]] = insertvalue [[STRUCT_UINT16X8X2_T]] [[TMP2]], <8 x i16> [[TMP3]], 0, 1
+// CHECK-NEXT:store <8 x i16> [[TMP1]], <8 x i16>* [[VALUES:%.*]], align 8
+// CHECK-NEXT:[[ARRAYIDX4:%.*]] = getelementptr inbounds <8 x i16>, <8 x i16>* [[VALUES]], i32 1
+// CHECK-NEXT:store <8 x i16> [[TMP3]], <8 x i16>* [[ARRAYIDX4]], align 8
+// CHECK-NEXT:ret void
+//
+void load_via_variable(const uint16_t *addr, uint16x8_t *values)
+{
+uint16x8x2_t v;
+#ifdef POLYMORPHIC
+v = vld2q(addr);
+#else /* POLYMORPHIC */
+v = vld2q_u16(addr);
+#endif /* PO

[PATCH] D71510: [clang][checkers] Added new checker 'error-return-checker'. (WIP)

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

Great job, this seems to be progressing nicely! please see my comments inline.




Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:38
+  // See if the result value from the system function (to check) is checked for
+  // error after a branch condition. 'Value' contains the (mostly conjured)
+  // symbolic value of the function call. 'RetTy' is the return type of the

Value is not the name of the parameter, maybe a refactoring missed this comment.



Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:77
+// This should work with any type of null value.
+// FIXME: Is this different from the ValueErrorResultChecker with 0 as value?
+class NullErrorResultChecker : public CheckForErrorResultChecker {

I think the current implementation is slightly different. The 
ValueErrorResultChecker uses symbolic evaluation of the equation with a 
ConcreteInt 0 value, while the ProgramState::isNull checks if the value is a 
constant and if it is not a zero constant (also calls further to 
ConstraintManager::isNull).



Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:158
+// value < 0" check too.
+class NegativeOrEofErrorResultChecker : public CheckForErrorResultChecker {
+public:

Maybe only checking for negative value is enough? My gut feeling is that the 
equality check is redundant. I would argue:
let A be: Value is negative
let B be: Value is equal to -1

since B implies A, (A or B) can be simplified to just A.
This presupposes that every time B can reasoned about A can be too.
This seems to be the case for here as well, as the definiteness of being equal 
to -1 is stronger than being lower than 0.




Comment at: clang/test/Analysis/error-return.c:270
+
+These are OK if not checked:
+

just for the sake of completeness; mention all the cases where error checking 
is deemed unnecessary as per ERR-33-EX1

https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors#ERR33-C.Detectandhandlestandardlibraryerrors-Exceptions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71510



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:3995
+``__attribute__((patchable_function_entry(N,M)))`` is used to generate M NOPs
+before the function entry and N-M NOPs after the function entry. This 
attributes
+takes precedence over command line option ``-fpatchable-function-entry=N,M``.

Grammar nits:
s/attributes/attribute/
over _the_ command line option



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:826
+  "patchable-function-entry",
+  (Twine(Attr->getSize()) + "," + Twine(Attr->getStart())).str());
   }

I think using two function attributes here would be better, to avoid needing to 
parse this again later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D70765: LTOVisibility.rst: fix up syntax in example

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D70765#1802564 , @dankamongmen 
wrote:

> I hate to bother anyone, but can this go ahead and get merged? :) thanks


Typically the author commits the patch, unless they don't have commit access 
and request the reviewer to commit for them, but I don't see a request for that 
here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70765



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

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D69878#1805804 , @arsenm wrote:

> In D69878#1801508 , 
> @cameron.mcinally wrote:
>
> > This is looking pretty good to me, but I'm ignoring some of the target 
> > specific code that I'm not familiar with.
> >
> > Is `denormal-fp-math` influenced by `-Ofast`? Or are there plans for that? 
> > Seems like `-Ofast` should imply DAZ and FTZ (if supported by target).
>
>
> Yes, through the toolchain handling. I copied the logic for when crtfastmath 
> is linked for the default mode for x86.
>
> > I think we discussed this before, but it's worth repeating. If 
> > `denormal-fp-math` isn't specified, we default to IEEE behavior, right? 
> > When this lands in master, there could be an unexpected performance hit for 
> > targets that aren't paying attention. E.g. I want to use `denormal-fp-math` 
> > to toggle whether a FSUB(-0.0,X) is converted to a FNEG(X) in 
> > SelectionDAGBuilder.
> > 
> > Apologies in advance if this has been discussed recently. I've been 
> > distracted with another project for the passed few months...
>
> Yes, ieee should be the default. The dependent patches start adding the 
> attribute by default for platforms with flushing enabled with fast math


To clarify this patch leaves the default and defers changing that to a later 
patch


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

https://reviews.llvm.org/D69878



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

[PATCH] D71365: expand printf when compiling HIP to AMDGPU

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/test/CodeGenHIP/printf.cpp:18
+}
+
+// CHECK: [[BEGIN:%.*]]   = call i64 @__ockl_printf_begin(i64 0)

This could use a lot more testcases. Can you add some half, float, and double 
as well as pointers (including different address spaces) and vectors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71365



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

[PATCH] D72053: [RFC] Handling implementation limits

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/docs/ImplementationQuantities.rst:11
+This page lists the limits implemented in the Clang compiler. The available
+resources on the system running Clang may imposse other limits. For example,
+the system may have insufficient memory to compile the translation unit before

Typo: imposse -> impose.


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

https://reviews.llvm.org/D72053



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

[PATCH] D72274: [libTooling] Fix bug in Stencil handling of macro ranges

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.

Currently, an attempt to rewrite source code inside a macro expansion succeeds, 
but results in empty text, rather than failing with an error.  This patch 
restructures to the code to explicitly validate ranges before attempting to 
edit them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72274

Files:
  clang/include/clang/Tooling/Transformer/SourceCode.h
  clang/lib/Tooling/Transformer/SourceCode.cpp
  clang/lib/Tooling/Transformer/Stencil.cpp
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/StencilTest.cpp

Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -368,6 +368,21 @@
   testExpr(Id, "3;", run(SimpleFn), "Bound");
 }
 
+TEST_F(StencilTest, CatOfInvalidRangeFails) {
+  StringRef Snippet = R"cpp(
+#define MACRO (3.77)
+  double foo(double d);
+  foo(MACRO);)cpp";
+
+  auto StmtMatch =
+  matchStmt(Snippet, callExpr(callee(functionDecl(hasName("foo"))),
+  argumentCountIs(1),
+  hasArgument(0, expr().bind("arg";
+  ASSERT_TRUE(StmtMatch);
+  Stencil S = cat(node("arg"));
+  EXPECT_THAT_EXPECTED(S->eval(StmtMatch->Result), Failed());
+}
+
 TEST(StencilToStringTest, RawTextOp) {
   auto S = cat("foo bar baz");
   StringRef Expected = R"("foo bar baz")";
Index: clang/unittests/Tooling/SourceCodeTest.cpp
===
--- clang/unittests/Tooling/SourceCodeTest.cpp
+++ clang/unittests/Tooling/SourceCodeTest.cpp
@@ -10,16 +10,20 @@
 #include "TestVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "llvm/Testing/Support/Annotations.h"
+#include "llvm/Testing/Support/Error.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include 
 #include 
 
 using namespace clang;
 
+using llvm::Failed;
+using llvm::Succeeded;
 using llvm::ValueIs;
 using tooling::getExtendedText;
 using tooling::getRangeForEdit;
 using tooling::getText;
+using tooling::validateEditRange;
 
 namespace {
 
@@ -200,4 +204,116 @@
   Visitor.runOver(Code.code());
 }
 
+TEST(SourceCodeTest, EditRangeWithMacroExpansionsIsValid) {
+  // The call expression, whose range we are extracting, includes two macro
+  // expansions.
+  llvm::StringRef Code = R"cpp(
+#define M(a) a * 13
+int foo(int x, int y);
+int a = foo(M(1), M(2));
+)cpp";
+
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
+EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+  Succeeded());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, SpellingRangeOfMacroArgIsValid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+SourceLocation ArgLoc =
+Context->getSourceManager().getSpellingLoc(Expr->getBeginLoc());
+// The integer literal is a single token.
+auto ArgRange = CharSourceRange::getTokenRange(ArgLoc);
+EXPECT_THAT_ERROR(validateEditRange(ArgRange, Context->getSourceManager()),
+  Succeeded());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvalidEditRangeIsInvalid) {
+  llvm::StringRef Code = "int c = 10;";
+
+  // We use the visitor just to get a valid context.
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *, ASTContext *Context) {
+CharSourceRange Invalid;
+EXPECT_THAT_ERROR(validateEditRange(Invalid, Context->getSourceManager()),
+  Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, InvertedEditRangeIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+int foo(int x);
+int a = foo(2);
+)cpp";
+
+  CallsVisitor Visitor;
+  Visitor.OnCall = [](CallExpr *Expr, ASTContext *Context) {
+auto InvertedRange = CharSourceRange::getTokenRange(
+SourceRange(Expr->getEndLoc(), Expr->getBeginLoc()));
+EXPECT_THAT_ERROR(
+validateEditRange(InvertedRange, Context->getSourceManager()),
+Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, MacroArgIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO(a) a + 7.0;
+int a = FOO(10);
+)cpp";
+
+  IntLitVisitor Visitor;
+  Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
+auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
+EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
+  Failed());
+  };
+  Visitor.runOver(Code);
+}
+
+TEST(SourceCodeTest, EditWholeMacroExpansionIsInvalid) {
+  llvm::StringRef Code = R"cpp(
+#define FOO 10
+int a = FOO;
+)cpp";
+
+  IntLitVisitor Visitor

[PATCH] D72270: [ARM,MVE] Fix many signedness errors in MVE intrinsics.

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.



Comment at: clang/include/clang/Basic/arm_mve.td:204
 let params = T.Float in {
-  defm vminnmq : VectorVectorArithmetic<"min_predicated">;
-  defm vmaxnmq : VectorVectorArithmetic<"max_predicated">;
+  defm vminnmq : VectorVectorArithmetic<"min_predicated", (? (u32 0))>;
+  defm vmaxnmq : VectorVectorArithmetic<"max_predicated", (? (u32 0))>;

What do these 0's mean?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72270



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

[PATCH] D72270: [ARM,MVE] Fix many signedness errors in MVE intrinsics.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked an inline comment as done.
simon_tatham added inline comments.



Comment at: clang/include/clang/Basic/arm_mve.td:204
 let params = T.Float in {
-  defm vminnmq : VectorVectorArithmetic<"min_predicated">;
-  defm vmaxnmq : VectorVectorArithmetic<"max_predicated">;
+  defm vminnmq : VectorVectorArithmetic<"min_predicated", (? (u32 0))>;
+  defm vmaxnmq : VectorVectorArithmetic<"max_predicated", (? (u32 0))>;

dmgreen wrote:
> What do these 0's mean?
The IR intrinsic `int_arm_mve_min_predicated` is used for both integer min/max 
and floating-point minnm/maxnm. For the integer case it needs a signed/unsigned 
flag parameter; so it has to have that parameter in the floating-point case as 
well, even though there's nothing to distinguish.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72270



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

[PATCH] D72276: [clang-format] Add IndentCaseBlocks option

[Nicolas Capens via Phabricator via cfe-commits]

capn created this revision.
capn added a reviewer: clang-format.
capn added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The documentation for IndentCaseLabels claimed that the "Switch
statement body is always indented one level more than case labels". This
is technically false for the code block immediately following the label.
Its closing bracket aligns with the start of the label.

If the case label are not indented, it leads to a style where the
closing bracket of the block aligns with the closing bracket of the
switch statement, which can be hard to parse.

This change introduces a new option, IndentCaseBlocks, which when true
treats the block as a scope block (which it technically is).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72276

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1211,6 +1211,28 @@
"  }\n"
"}",
Style));
+  Style.IndentCaseLabels = false;
+  Style.IndentCaseBlocks = true;
+  EXPECT_EQ("switch (n)\n"
+"{\n"
+"case 0:\n"
+"  {\n"
+"return false;\n"
+"  }\n"
+"default:\n"
+"  {\n"
+"return true;\n"
+"  }\n"
+"}",
+format("switch (n) {\n"
+   "case 0: {\n"
+   "  return false;\n"
+   "}\n"
+   "default: {\n"
+   "  return true;\n"
+   "}\n"
+   "}",
+   Style));
 }
 
 TEST_F(FormatTest, CaseRanges) {
@@ -12578,6 +12600,7 @@
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
+  CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1985,7 +1985,8 @@
 --Line->Level;
   if (LeftAlignLabel)
 Line->Level = 0;
-  if (CommentsBeforeNextToken.empty() && FormatTok->Tok.is(tok::l_brace)) {
+  if (!Style.IndentCaseBlocks && CommentsBeforeNextToken.empty() &&
+  FormatTok->Tok.is(tok::l_brace)) {
 CompoundStatementIndenter Indenter(this, Line->Level,
Style.BraceWrapping.AfterCaseLabel,
Style.BraceWrapping.IndentBraces);
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -480,6 +480,7 @@
 IO.mapOptional("IncludeIsMainSourceRegex",
Style.IncludeStyle.IncludeIsMainSourceRegex);
 IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
+IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
 IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
 IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
 IO.mapOptional("IndentWidth", Style.IndentWidth);
@@ -782,6 +783,7 @@
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
   LLVMStyle.IndentCaseLabels = false;
+  LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1314,7 +1314,8 @@
   ///
   /// When ``false``, use the same indentation level as for the switch
   /// statement. Switch statement body is always indented one level more than
-  /// case labels.
+  /// case labels (except the first block following the case label, which
+  /// itself indents the code - unless IndentCaseBlocks is enabled).
   /// \code
   ///false: true:
   ///switch (fool) {vs. switch (fool) {
@@ -1327,6 +1328,28 @@
   /// \endcode
   bool IndentCaseLabels;
 
+  /// Indent case label blocks one level from the case label.
+  ///
+  /// When ``false``, the block following the case label uses the same
+  /// indentation level as for the case label, treating the case label the

[PATCH] D72270: [ARM,MVE] Fix many signedness errors in MVE intrinsics.

[Dave Green via Phabricator via cfe-commits]

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

OK. LGTM then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72270



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

[PATCH] D72268: [ARM,MVE] Support -ve offsets in gather-load intrinsics.

[Dave Green via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72268



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

[clang] 4978296 - [ARM, MVE] Support -ve offsets in gather-load intrinsics.

[Simon Tatham via cfe-commits]

Author: Simon Tatham
Date: 2020-01-06T16:33:07Z
New Revision: 4978296cd8e4d10724cfa41f0308d256c0fd490c

URL: 
https://github.com/llvm/llvm-project/commit/4978296cd8e4d10724cfa41f0308d256c0fd490c
DIFF: 
https://github.com/llvm/llvm-project/commit/4978296cd8e4d10724cfa41f0308d256c0fd490c.diff

LOG: [ARM,MVE] Support -ve offsets in gather-load intrinsics.

Summary:
The ACLE intrinsics with `gather_base` or `scatter_base` in the name
are wrappers on the MVE load/store instructions that take a vector of
base addresses and an immediate offset. The immediate offset can be up
to 127 times the alignment unit, and it can be positive or negative.

At the MC layer, we got that right. But in the Sema error checking for
the wrapping intrinsics, the offset was erroneously constrained to be
positive.

To fix this I've adjusted the `imm_mem7bit` class in the Tablegen that
defines the intrinsics. But that causes integer literals like
`0xfe04` to appear in the autogenerated calls to
`SemaBuiltinConstantArgRange`, which provokes a compiler warning
because that's out of the non-overflowing range of an `int64_t`. So
I've also tweaked `MveEmitter` to emit that as `-0x1fc` instead.

Updated the tests of the Sema checks themselves, and also adjusted a
random sample of the CodeGen tests to actually use negative offsets
and prove they get all the way through code generation without causing
a crash.

Reviewers: dmgreen, miyuki, MarkMurrayARM

Reviewed By: dmgreen

Subscribers: kristof.beyls, cfe-commits, llvm-commits

Tags: #clang, #llvm

Differential Revision: https://reviews.llvm.org/D72268

Added: 


Modified: 
clang/include/clang/Basic/arm_mve_defs.td
clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
clang/test/Sema/arm-mve-immediates.c
clang/utils/TableGen/MveEmitter.cpp
llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll

Removed: 




diff  --git a/clang/include/clang/Basic/arm_mve_defs.td 
b/clang/include/clang/Basic/arm_mve_defs.td
index 939d5eb0cd6b..6fba88df34bf 100644
--- a/clang/include/clang/Basic/arm_mve_defs.td
+++ b/clang/include/clang/Basic/arm_mve_defs.td
@@ -345,9 +345,10 @@ def imm_1248 : Immediate> {
 
 // imm_mem7bit is a valid immediate offset for a load/store intrinsic whose
 // memory access size is n bytes (e.g. 1 for vldrb_[whatever], 2 for vldrh,
-// ...). The set of valid immediates for these is {0*n, 1*n, ..., 127*n}.
+// ...). The set of valid immediates for these is {-127*n, ..., -1*n, 0*n, 1*n,
+// ..., 127*n}.
 class imm_mem7bit
-  : Immediate> {
+  : Immediate> {
   let extra = !if(!eq(membytes, 1), ?, "Multiple");
   let extraarg = !cast(membytes);
 }

diff  --git a/clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c 
b/clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
index 8bf2111a9e63..564965acc04d 100644
--- a/clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
+++ b/clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
@@ -196,12 +196,12 @@ int64x2_t test_vldrdq_gather_base_s64(uint64x2_t addr)
 
 // CHECK-LABEL: @test_vldrdq_gather_base_u64(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:[[TMP0:%.*]] = call <2 x i64> 
@llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> [[ADDR:%.*]], i32 336)
+// CHECK-NEXT:[[TMP0:%.*]] = call <2 x i64> 
@llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> [[ADDR:%.*]], i32 -336)
 // CHECK-NEXT:ret <2 x i64> [[TMP0]]
 //
 uint64x2_t test_vldrdq_gather_base_u64(uint64x2_t addr)
 {
-return vldrdq_gather_base_u64(addr, 0x150);
+return vldrdq_gather_base_u64(addr, -0x150);
 }
 
 // CHECK-LABEL: @test_vldrdq_gather_base_wb_s64(
@@ -221,7 +221,7 @@ int64x2_t test_vldrdq_gather_base_wb_s64(uint64x2_t *addr)
 // CHECK-LABEL: @test_vldrdq_gather_base_wb_u64(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[TMP0:%.*]] = load <2 x i64>, <2 x i64>* [[ADDR:%.*]], 
align 8
-// CHECK-NEXT:[[TMP1:%.*]] = call { <2 x i64>, <2 x i64> } 
@llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> [[TMP0]], i32 328)
+// CHECK-NEXT:[[TMP1:%.*]] = call { <2 x i64>, <2 x i64> } 
@llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> [[TMP0]], i32 -328)
 // CHECK-NEXT:[[TMP2:%.*]] = extractvalue { <2 x i64>, <2 x i64> } 
[[TMP1]], 1
 // CHECK-NEXT:store <2 x i64> [[TMP2]], <2 x i64>* [[ADDR]], align 8
 // CHECK-NEXT:[[TMP3:%.*]] = extractvalue { <2 x i64>, <2 x i64> } 
[[TMP1]], 0
@@ -229,7 +229,7 @@ int64x2_t test_vldrdq_gather_base_wb_s64(uint64x2_t *addr)
 //
 uint64x2_t test_vldrdq_gather_base_wb_u64(uint64x2_t *addr)
 {
-return vldrdq_gather_base_wb_u64(addr, 0x148);
+return vldrdq_gather_base_wb_u64(addr, -0x148);
 }
 
 // CHECK-LABEL: @test_vldrdq_gather_base_wb_z_s64(
@@ -280,12 +280,12 @@ int64x2_t test_vldrdq_gather_base_z_s64(uint64x2_t addr, 
mve_pred16_t p)
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[TMP0:%.*]] = zext i16 [[P:%.*]] to i32
 // CHECK-NEXT:[[TMP1:%.*]] = call <4 x i1> @llvm.ar

[PATCH] D72268: [ARM,MVE] Support -ve offsets in gather-load intrinsics.

[Simon Tatham via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4978296cd8e4: [ARM,MVE] Support -ve offsets in gather-load 
intrinsics. (authored by simon_tatham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72268

Files:
  clang/include/clang/Basic/arm_mve_defs.td
  clang/test/CodeGen/arm-mve-intrinsics/scatter-gather.c
  clang/test/Sema/arm-mve-immediates.c
  clang/utils/TableGen/MveEmitter.cpp
  llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll

Index: llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
===
--- llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
+++ llvm/test/CodeGen/Thumb2/mve-intrinsics/scatter-gather.ll
@@ -191,11 +191,11 @@
 define arm_aapcs_vfpcc <2 x i64> @test_vldrdq_gather_base_u64(<2 x i64> %addr) {
 ; CHECK-LABEL: test_vldrdq_gather_base_u64:
 ; CHECK:   @ %bb.0: @ %entry
-; CHECK-NEXT:vldrd.u64 q1, [q0, #336]
+; CHECK-NEXT:vldrd.u64 q1, [q0, #-336]
 ; CHECK-NEXT:vmov q0, q1
 ; CHECK-NEXT:bx lr
 entry:
-  %0 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> %addr, i32 336)
+  %0 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.v2i64.v2i64(<2 x i64> %addr, i32 -336)
   ret <2 x i64> %0
 }
 
@@ -221,12 +221,12 @@
 ; CHECK-LABEL: test_vldrdq_gather_base_wb_u64:
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
-; CHECK-NEXT:vldrd.u64 q1, [q0, #328]!
+; CHECK-NEXT:vldrd.u64 q1, [q0, #-328]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <2 x i64>, <2 x i64>* %addr, align 8
-  %1 = call { <2 x i64>, <2 x i64> } @llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> %0, i32 328)
+  %1 = call { <2 x i64>, <2 x i64> } @llvm.arm.mve.vldr.gather.base.wb.v2i64.v2i64(<2 x i64> %0, i32 -328)
   %2 = extractvalue { <2 x i64>, <2 x i64> } %1, 1
   store <2 x i64> %2, <2 x i64>* %addr, align 8
   %3 = extractvalue { <2 x i64>, <2 x i64> } %1, 0
@@ -297,13 +297,13 @@
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vmsr p0, r0
 ; CHECK-NEXT:vpst
-; CHECK-NEXT:vldrdt.u64 q1, [q0, #1000]
+; CHECK-NEXT:vldrdt.u64 q1, [q0, #-1000]
 ; CHECK-NEXT:vmov q0, q1
 ; CHECK-NEXT:bx lr
 entry:
   %0 = zext i16 %p to i32
   %1 = call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %0)
-  %2 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.predicated.v2i64.v2i64.v4i1(<2 x i64> %addr, i32 1000, <4 x i1> %1)
+  %2 = call <2 x i64> @llvm.arm.mve.vldr.gather.base.predicated.v2i64.v2i64.v4i1(<2 x i64> %addr, i32 -1000, <4 x i1> %1)
   ret <2 x i64> %2
 }
 
@@ -728,12 +728,12 @@
 ; CHECK-LABEL: test_vldrwq_gather_base_wb_f32:
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
-; CHECK-NEXT:vldrw.u32 q1, [q0, #64]!
+; CHECK-NEXT:vldrw.u32 q1, [q0, #-64]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <4 x i32>, <4 x i32>* %addr, align 8
-  %1 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.v4f32.v4i32(<4 x i32> %0, i32 64)
+  %1 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.v4f32.v4i32(<4 x i32> %0, i32 -64)
   %2 = extractvalue { <4 x float>, <4 x i32> } %1, 1
   store <4 x i32> %2, <4 x i32>* %addr, align 8
   %3 = extractvalue { <4 x float>, <4 x i32> } %1, 0
@@ -782,14 +782,14 @@
 ; CHECK-NEXT:vmsr p0, r1
 ; CHECK-NEXT:vldrw.u32 q0, [r0]
 ; CHECK-NEXT:vpst
-; CHECK-NEXT:vldrwt.u32 q1, [q0, #352]!
+; CHECK-NEXT:vldrwt.u32 q1, [q0, #-352]!
 ; CHECK-NEXT:vstrw.32 q1, [r0]
 ; CHECK-NEXT:bx lr
 entry:
   %0 = load <4 x i32>, <4 x i32>* %addr, align 8
   %1 = zext i16 %p to i32
   %2 = call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %1)
-  %3 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.predicated.v4f32.v4i32.v4i1(<4 x i32> %0, i32 352, <4 x i1> %2)
+  %3 = call { <4 x float>, <4 x i32> } @llvm.arm.mve.vldr.gather.base.wb.predicated.v4f32.v4i32.v4i1(<4 x i32> %0, i32 -352, <4 x i1> %2)
   %4 = extractvalue { <4 x float>, <4 x i32> } %3, 1
   store <4 x i32> %4, <4 x i32>* %addr, align 8
   %5 = extractvalue { <4 x float>, <4 x i32> } %3, 0
@@ -845,13 +845,13 @@
 ; CHECK:   @ %bb.0: @ %entry
 ; CHECK-NEXT:vmsr p0, r0
 ; CHECK-NEXT:vpst
-; CHECK-NEXT:vldrwt.u32 q1, [q0, #300]
+; CHECK-NEXT:vldrwt.u32 q1, [q0, #-300]
 ; CHECK-NEXT:vmov q0, q1
 ; CHECK-NEXT:bx lr
 entry:
   %0 = zext i16 %p to i32
   %1 = call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %0)
-  %2 = call <4 x float> @llvm.arm.mve.vldr.gather.base.predicated.v4f32.v4i32.v4i1(<4 x i32> %addr, i32 300, <4 x i1> %1)
+  %2 = call <4 x float> @llvm.arm.mve.vldr.gather.base.predicated.v4f32.v4i32.v4i1(<4 x i32> %addr, i32 -300, <4 x i1> %1)
   ret <4 x float> %2
 }
 
@@ -1254,10 +1254,10 @@
 define arm_aapcs_vfpcc void @test_vstrdq_scatter_base_u64(<2 x i64> %addr, <2 x i64> 

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Cameron McInally via Phabricator via cfe-commits]

cameron.mcinally added a comment.

Ok, thanks for the clarifications. Looks good to me, but it would be good to 
have experts in OpenCL/Cuda/AMDGPU review the target specific changes.


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

https://reviews.llvm.org/D69878



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

[PATCH] D72053: [RFC] Handling implementation limits

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.



Comment at: clang/docs/ImplementationQuantities.rst:11
+This page lists the limits implemented in the Clang compiler. The available
+resources on the system running Clang may imposse other limits. For example,
+the system may have insufficient memory to compile the translation unit before

tahonermann wrote:
> Typo: imposse -> impose.
Thanks, it will be updated in the next revision of the patch.


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

https://reviews.llvm.org/D72053



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

[PATCH] D72281: [Matrix] Add matrix type to Clang (WIP).

[Florian Hahn via Phabricator via cfe-commits]

fhahn created this revision.
Herald added a reviewer: martong.
Herald added subscribers: tschuett, arphaman.
Herald added a project: clang.

This patch adds a matrix type to Clang as described in
"Matrix Support in Clang" on cfe-dev [1]. The patch is not intended for
review yet, just to provide an idea how the implementation would look
like.

One aspect in particular I would appreciate feedback on is how to best
ensure matrix type values are aligned the same as pointers to the
element type, while using LLVM's vector type to lower operations.

The main problem is struct layouting, where LLVM's vector type has a
larger alignment than desired.

To work around that fact, the patch uses array types as storage types for
matrix values, but vector types in other contexts. After loading/before
storing, we bitcast between array type and vector type. Alternatively
we could opt for generating packed LLVM structs.

The builtins will be added in separate, follow-on patches.

[1] http://lists.llvm.org/pipermail/cfe-dev/2019-December/064141.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72281

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/Type.h
  clang/include/clang/AST/TypeLoc.h
  clang/include/clang/AST/TypeProperties.td
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TypeNodes.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/TypeBitCodes.def
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/matrix-type.c
  clang/test/CodeGenCXX/matrix-type.cpp
  clang/test/SemaCXX/matrix-type.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -1792,6 +1792,8 @@
 DEFAULT_TYPELOC_IMPL(DependentSizedExtVector, Type)
 DEFAULT_TYPELOC_IMPL(Vector, Type)
 DEFAULT_TYPELOC_IMPL(ExtVector, VectorType)
+DEFAULT_TYPELOC_IMPL(Matrix, Type)
+DEFAULT_TYPELOC_IMPL(DependentSizedMatrix, Type)
 DEFAULT_TYPELOC_IMPL(FunctionProto, FunctionType)
 DEFAULT_TYPELOC_IMPL(FunctionNoProto, FunctionType)
 DEFAULT_TYPELOC_IMPL(Record, TagType)
Index: clang/test/SemaCXX/matrix-type.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/matrix-type.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -pedantic -fenable-matrix -std=c++11 -verify -triple x86_64-apple-darwin %s
+
+using matrix_double_t = double __attribute__((matrix_type(6, 6)));
+using matrix_float_t = float __attribute__((matrix_type(6, 6)));
+using matrix_int_t = int __attribute__((matrix_type(6, 6)));
+
+void matrix_var_dimensions(int Rows, unsigned Columns, char C) {
+  using matrix1_t = int __attribute__((matrix_type(Rows, 1)));// expected-error{{matrix_type attribute requires an integer constant}}
+  using matrix2_t = int __attribute__((matrix_type(1, Columns))); // expected-error{{matrix_type attribute requires an integer constant}}
+  using matrix3_t = int __attribute__((matrix_type(C, C)));   // expected-error{{matrix_type attribute requires an integer constant}}
+  using matrix4_t = int __attribute__((matrix_type(-1, 1)));  // expected-error{{vector size too large}}
+  using matrix5_t = int __attribute__((matrix_type(1, -1)));  // expected-error{{vector size too large}}
+  using matrix6_t = int __attribute__((matrix_type(0, 1)));   // expected-error{{zero vector size}}
+  using matrix7_t = int __attribute__((matrix_type(1, 0)));   // expected-error{{zero vector size}}
+  using matrix7_t = int __attribute__((matrix_type(char, 0)));// expected-error{{expected '(' for function-style cast or type construction}}
+}
+
+struct S1 {};
+
+void matrix_unsupported_element_type() {
+  using matrix1_t = char *__attribute__((matrix_type(1, 1))); // expected-error{{invalid matrix element type 'char *'}}
+  using matrix2_t = S1 __attribute__((matrix_type(1, 1)));// expected-error{{invalid matrix element type 

[PATCH] D72283: [Matrix] Add __builtin_matrix_insert to Clang (WIP).

[Florian Hahn via Phabricator via cfe-commits]

fhahn created this revision.
Herald added a subscriber: tschuett.
Herald added a project: clang.

This patch adds a __builtin_matrix_insert builtin as described in
"Matrix Support in Clang" on cfe-dev [1]. The patch is not intended for
review yet, just to provide an idea how the implementation could look
like.

[1] http://lists.llvm.org/pipermail/cfe-dev/2019-December/064141.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72283

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-matrix.c
  clang/test/CodeGenCXX/builtin-matrix.cpp
  clang/test/SemaCXX/builtin-matrix.cpp

Index: clang/test/SemaCXX/builtin-matrix.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-matrix.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 %s -fenable-matrix -pedantic -std=c++11 -verify -triple=x86_64-apple-darwin9
+
+typedef float sx10x10_t __attribute__((matrix_type(10, 10)));
+sx10x10_t a;
+
+struct Foo {
+  char *s;
+};
+
+void insert(sx10x10_t *a, float f) {
+  *a = __builtin_matrix_insert(
+  10, // expected-error {{First argument must be a matrix}}
+  a,  // expected-error {{Row argument must be an unsigned integer}}
+  a,  // expected-error {{Column argument must be an unsigned integer}}
+  10);
+
+  int x = __builtin_matrix_insert(*a, 3u, 5u, 10.0); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'sx10x10_t' (aka 'float __attribute__((matrix_type(10, 10))) ')}}
+
+  // TODO: Should error here (index out of range).
+  *a = __builtin_matrix_insert(*a, -1u, 5u, 10.0);
+
+  // FIXME: Column argument is fine!
+  *a = __builtin_matrix_insert(*a, f, // expected-error {{Row argument must be an unsigned integer}}
+   5u, 10.0); // expected-error {{Column argument must be an unsigned integer}}
+}
Index: clang/test/CodeGenCXX/builtin-matrix.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/builtin-matrix.cpp
@@ -0,0 +1,150 @@
+// RUN: %clang_cc1 -fenable-matrix -triple x86_64-apple-darwin %s -emit-llvm -disable-llvm-passes -o - -std=c++11 | FileCheck %s
+
+typedef double dx5x5_t __attribute__((matrix_type(5, 5)));
+using fx2x3_t = float __attribute__((matrix_type(2, 3)));
+
+void insert_fp(dx5x5_t *a, double d, fx2x3_t *b, float e) {
+  *a = __builtin_matrix_insert(*a, 0u, 1u, d);
+  *b = __builtin_matrix_insert(*b, 1u, 0u, e);
+
+  // CHECK-LABEL: @_Z9insert_fpPDm5_5_ddPDm2_3_ff(
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT:%a.addr = alloca [25 x double]*, align 8
+  // CHECK-NEXT:%d.addr = alloca double, align 8
+  // CHECK-NEXT:%b.addr = alloca [6 x float]*, align 8
+  // CHECK-NEXT:%e.addr = alloca float, align 4
+  // CHECK-NEXT:store [25 x double]* %a, [25 x double]** %a.addr, align 8
+  // CHECK-NEXT:store double %d, double* %d.addr, align 8
+  // CHECK-NEXT:store [6 x float]* %b, [6 x float]** %b.addr, align 8
+  // CHECK-NEXT:store float %e, float* %e.addr, align 4
+  // CHECK-NEXT:%0 = load [25 x double]*, [25 x double]** %a.addr, align 8
+  // CHECK-NEXT:%1 = bitcast [25 x double]* %0 to <25 x double>*
+  // CHECK-NEXT:%2 = load <25 x double>, <25 x double>* %1, align 8
+  // CHECK-NEXT:%3 = load double, double* %d.addr, align 8
+  // CHECK-NEXT:%4 = insertelement <25 x double> %2, double %3, i32 5
+  // CHECK-NEXT:%5 = load [25 x double]*, [25 x double]** %a.addr, align 8
+  // CHECK-NEXT:%6 = bitcast [25 x double]* %5 to <25 x double>*
+  // CHECK-NEXT:store <25 x double> %4, <25 x double>* %6, align 8
+  // CHECK-NEXT:%7 = load [6 x float]*, [6 x float]** %b.addr, align 8
+  // CHECK-NEXT:%8 = bitcast [6 x float]* %7 to <6 x float>*
+  // CHECK-NEXT:%9 = load <6 x float>, <6 x float>* %8, align 4
+  // CHECK-NEXT:%10 = load float, float* %e.addr, align 4
+  // CHECK-NEXT:%11 = insertelement <6 x float> %9, float %10, i32 1
+  // CHECK-NEXT:%12 = load [6 x float]*, [6 x float]** %b.addr, align 8
+  // CHECK-NEXT:%13 = bitcast [6 x float]* %12 to <6 x float>*
+  // CHECK-NEXT:store <6 x float> %11, <6 x float>* %13, align 4
+  // CHECK-NEXT:   ret void
+}
+
+typedef int ix9x3_t __attribute__((matrix_type(9, 3)));
+
+void insert_int(ix9x3_t *a, int i) {
+  *a = __builtin_matrix_insert(*a, 4u, 1u, i);
+
+  // CHECK-LABEL: @_Z10insert_intPDm9_3_ii(
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT:%a.addr = alloca [27 x i32]*, align 8
+  // CHECK-NEXT:%i.addr = alloca i32, align 4
+  // CHECK-NEXT:store [27 x i32]* %a, [27 x i32]** %a.addr, align 8
+  // CHECK-NEXT:store i32 %i, i32* %i.addr, align 4
+  // CHECK-NEXT:%0 = load [27 x i32]*, [27 x i32]** %a.addr, align 8
+  // CHECK-NEXT:%1 = bitcast [27 x i32]* %0 to

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Logan Smith via Phabricator via cfe-commits]

logan-5 created this revision.
logan-5 added reviewers: aaron.ballman, hokein, alexfh.
logan-5 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.

This patch adds `bugprone-unintended-adl` which flags uses of ADL that are not 
on the provided whitelist.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72282

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp
@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy %s bugprone-unintended-adl %t
+
+namespace aspace {
+struct A {};
+void func(const A &);
+} // namespace aspace
+
+namespace bspace {
+void func(int);
+void test() {
+  aspace::A a;
+  func(5);
+  func(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'aspace::func' through ADL [bugprone-unintended-adl]
+}
+} // namespace bspace
+
+namespace ops {
+struct Stream {
+} stream;
+Stream &operator<<(Stream &s, int) {
+  return s;
+}
+void smooth_operator(Stream);
+} // namespace ops
+
+void test() {
+  ops::stream << 5;
+  operator<<(ops::stream, 5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl]
+  smooth_operator(ops::stream);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl]
+}
+
+namespace ns {
+struct Swappable {};
+void swap(Swappable &a, Swappable &b); // whitelisted by default
+
+void move(Swappable) {} // non-whitelisted
+template 
+void forward(T) {} // non-whitelisted
+
+struct Swappable2 {};
+} // namespace ns
+
+void move(ns::Swappable2);
+auto ref = [](ns::Swappable2) {};
+
+void test2() {
+  ns::Swappable a, b;
+  using namespace std;
+  swap(a, b);
+  move(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl]
+  forward(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::forward' through ADL [bugprone-unintended-adl]
+}
+
+template 
+void templateFunction(T t) {
+  swap(t, t);
+
+  move(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'move' may be resolved through ADL [bugprone-unintended-adl]
+
+  ns::move(t);
+  ::move(t);
+
+  ref(t);
+  ::ref(t);
+
+  t << 5;
+  operator<<(t, 5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'operator<<' may be resolved through ADL [bugprone-unintended-adl]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
@@ -0,0 +1,39 @@
+.. title:: clang-tidy - bugprone-unintended-adl
+
+bugprone-unintended-adl
+===
+
+Finds usages of ADL (argument-dependent lookup), or potential ADL in the case of templates, that are not on the provided whitelist.
+
+.. code-block:: c++
+
+  namespace aspace {
+  struct A {};
+  void func(const A &);
+  } // namespace aspace
+  
+  namespace bspace {
+  void func(int);
+  void test() {
+aspace::A a;
+func(5);
+func(a); // calls 'aspace::func' through ADL
+  }
+  } // namespace bspace
+
+(Example respectfully borrowed from `Abseil TotW #49 `_.)
+
+ADL can be surprising, and can lead to `subtle bugs `_ without the utmost attention. However, it very is useful for lookup of overloaded operators, and for customization points within libraries (e.g. `swap` in the C++ standard library). As such, this check can be configured to ignore calls to overloaded operators as well as other legitimate uses of ADL specified in a whitelist.
+
+This check does not suggest any fixes.
+
+Options
+---
+
+.. option:: IgnoreOverloadedOperators
+
+   If non-zero, ignores calls to overloaded operators using the operator syntax (e.g. `a + b`), but not the function call syntax (e.g. `operator+(a, b)`). Default is `1`.
+
+.. option:: Whitelist
+
+   Semicolon-separated list of names that the check ignores. Default is `swap`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,12 @@
   
   Violating the naming rules above results in undefined b

[PATCH] D72283: [Matrix] Add __builtin_matrix_insert to Clang (WIP).

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon question-circle color=gray} clang-tidy: unknown.

{icon question-circle color=gray} clang-format: unknown.

Build artifacts 
: 
diff.json 
,
 console-log.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72283



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

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D68101#1802220 , @bd1976llvm wrote:

> Below is the code comment from the new patch explaining the new approach, 
> please take a look and see if you have any questions/comments:
>
>   // If two globals with differing sizes end up in the same mergeable
>   // section that section can be assigned an incorrect entry size. Normally,
>   // the assembler avoids this by putting incompatible globals into
>   // differently named sections. However, globals can be explicitly assigned
>   // to a section by specifying the section name. In this case, if unique
>   // section names are available (-unique-section-names in LLVM) then we
>   // bin compatible globals into different mergeable sections with the same 
> name.


Looks good up to here.

>   // Otherwise, if incompatible globals have been explicitly assigned to 
> section by a
>   // fine-grained/per-symbol mechanism (e.g. via  
> _attribute_((section(“myname” then
>   // we issue an error and the user can then change the section assignment. 
> If the

No bueno.  The Linux kernel has code where there are 2D global arrays of 
different entity sizes explicitly placed in different sections (together). GCC 
does not error for this, (it doesn't mark the section merge-able), neither 
should we.

In D68101#1802280 , @rjmccall wrote:

> I laid out a series of three options before:
>
> - Emit different object-file sections for different mergeability settings.
> - Only mark an object-file section as mergeable if all the symbols in it 
> would have the same mergeability settings.
> - Stop implicitly using mergeability for "ordinary" sections (i.e. sections 
> other than the string section).


Of the above, I really think #2 is the only complete solution.


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

https://reviews.llvm.org/D68101



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

[PATCH] D69876: Allow output constraints on "asm goto"

[Nick Desaulniers via Phabricator via cfe-commits]

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

No, thanks for the work on this @void !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876



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

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

[Tamás Zolnai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG350da402ef6b: [clang-tidy] new check: 
bugprone-signed-char-misuse (authored by ztamas).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71174

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-signed-char-misuse.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-fsigned-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-funsigned-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse-with-option.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signed-char-misuse.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s bugprone-signed-char-misuse %t
+
+///
+/// Test cases correctly caught by the check.
+
+int SimpleVarDeclaration() {
+  signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int SimpleAssignment() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int CStyleCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = (int)CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int StaticCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = static_cast(CCharacter);
+  // CHECK-MESSAGES: [[@LINE-1]]:33: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int FunctionalCast() {
+  signed char CCharacter = -5;
+  int NCharacter;
+  NCharacter = int(CCharacter);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int NegativeConstValue() {
+  const signed char CCharacter = -5;
+  int NCharacter = CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+int CharPointer(signed char *CCharacter) {
+  int NCharacter = *CCharacter;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
+
+  return NCharacter;
+}
+
+///
+/// Test cases correctly ignored by the check.
+
+int UnsignedCharCast() {
+  unsigned char CCharacter = 'a';
+  int NCharacter = CCharacter;
+
+  return NCharacter;
+}
+
+int PositiveConstValue() {
+  const signed char CCharacter = 5;
+  int NCharacter = CCharacter;
+
+  return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of declaration expression.
+int DescendantCast() {
+  signed char CCharacter = 'a';
+  int NCharacter = 10 + CCharacter;
+
+  return NCharacter;
+}
+
+// singed char -> integer cast is not the direct child of assignment expression.
+int DescendantCastAssignment() {
+  signed char CCharacter = 'a';
+  int NCharacter;
+  NCharacter = 10 + CCharacter;
+
+  return NCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolVarDeclaration() {
+  signed char CCharacter = 'a';
+  bool BCharacter = CCharacter == 'b';
+
+  return BCharacter;
+}
+
+// bool is an integer type in clang; make sure to ignore it.
+bool BoolAssignment() {
+  signed char CCharacter = 'a';
+  bool BCharacter;
+  BCharacter = CCharacter == 'b';
+
+  return BCharacter;
+}
+
+// char is an integer type in clang; make sure to ignore it.
+unsigned char CharToCharCast() {
+  signed char SCCharacter = 'a';
+  unsigned char USCharacter;
+  USCharacter = SCCharacter;
+
+  return USCharacter;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugpro

[PATCH] D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming

[Logan Smith via Phabricator via cfe-commits]

logan-5 created this revision.
logan-5 added reviewers: alexfh, hokein, aaron.ballman.
logan-5 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.

Before this patch, `readability-identifier-naming` contained a significant 
amount of logic for (a) checking the style of identifiers, followed by (b) 
renaming/applying fix-its. This patch factors out (b) into a separate base 
class so that it can be reused by other checks that want to do renaming. This 
also cleans up `readability-identifier-naming` significantly, since now it only 
needs to be concerned with the interesting details of (a).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72284

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h

Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
@@ -0,0 +1,145 @@
+//===--- RenamderClangTidyCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_RENAMERCLANGTIDYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_RENAMERCLANGTIDYCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include 
+#include 
+
+namespace clang {
+
+class MacroInfo;
+
+namespace tidy {
+
+/// Base class for clang-tidy checks that want to flag declarations and/or
+/// macros for renaming based on customizable criteria.
+class RenamerClangTidyCheck : public ClangTidyCheck {
+public:
+  RenamerClangTidyCheck(StringRef CheckName, ClangTidyContext *Context);
+  ~RenamerClangTidyCheck();
+
+  /// Derived classes should not implement any matching logic themselves; this
+  /// class will do the matching and call the derived class'
+  /// GetDeclFailureInfo() and GetMacroFailureInfo() for determining whether a
+  /// given identifier passes or fails the check.
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override final;
+  void
+  check(const ast_matchers::MatchFinder::MatchResult &Result) override final;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override final;
+  void onEndOfTranslationUnit() override final;
+
+  /// This enum will be used in %select of the diagnostic message.
+  /// Each value below IgnoreFailureThreshold should have an error message.
+  enum class ShouldFixStatus {
+ShouldFix,
+ConflictsWithKeyword, /// The fixup will conflict with a language keyword,
+  /// so we can't fix it automatically.
+ConflictsWithMacroDefinition, /// The fixup will conflict with a macro
+  /// definition, so we can't fix it
+  /// automatically.
+
+/// Values pass this threshold will be ignored completely
+/// i.e no message, no fixup.
+IgnoreFailureThreshold,
+
+InsideMacro, /// If the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+  };
+
+  /// Information describing a failed check
+  struct FailureInfo {
+std::string KindName; // Tag or misc info to be used as derived classes need
+std::string Fixup;// The name that will be proposed as a fix-it hint
+  };
+
+  /// Holds an identifier name check failure, tracking the kind of the
+  /// identifier, its possible fixup and the starting locations of all the
+  /// identifier usages.
+  struct NamingCheckFailure {
+FailureInfo Info;
+
+/// Whether the failure should be fixed or not.
+///
+/// e.g.: if the identifier was used or declared within a macro we won't
+/// offer a fixup for safety reasons.
+bool ShouldFix() const {
+  return FixStatus == ShouldFixStatus::ShouldFix && !Info.Fixup.empty();
+}
+
+bool ShouldNotify() const {
+  return FixStatus < ShouldFixStatus::IgnoreFailureThreshold;
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
+
+/// A set of all the identifier usages starting SourceLocation, in
+/// their encoded form.
+llvm::DenseSet RawUsageLocs;
+
+NamingCheckFailure() = default;
+  };
+
+  typedef std::pair NamingCheckId;
+
+  typedef llvm::DenseMap
+  NamingCheckFailureMap;
+
+  /// Che

[PATCH] D71314: Emit a warning if a variable is uninitialized in indirect ASM goto destination.

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Analysis/UninitializedValues.cpp:856
+vals[VD] = MayUninitialized;
 }
 

Can you walk me through the logic of this function?

I would assume for changes to `asm goto`, the above early `return` would have 
been removed? Otherwise we seem to be changing the logic of the non-`asm goto` 
case?



Comment at: clang/test/Analysis/uninit-asm-goto.cpp:57
+  return y;
+indirect:
+  return -2;

nickdesaulniers wrote:
> I think if you left out `indirect`, it would be clearer what this test is 
> testing.
bump


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71314



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

[PATCH] D72213: [clang-tidy] Add `bugprone-reserved-identifier` to flag usages of reserved C++ names

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

I'm taking @JonasToth's suggestion and splitting this into two patches, one for 
the refactor, followed by one for the new check. The refactor patch is here: 
https://reviews.llvm.org/D72284. Thanks everyone for your feedback so far.


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

https://reviews.llvm.org/D72213



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

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4333
+if (!ReturnValue.isNull() &&
+
ReturnValue.getValue().getType()->getPointerElementType()->isStructTy())
+  return RValue::getAggregate(ReturnValue.getValue(),

The requirement is that we construct an `RValue` of the right kind for the 
expression's result type, so the correct approach here is to switch over 
`getEvaluationKind(E->getType())`.  This is also a more reliable signal than 
the presence of the return value slot.

We then need to think about how the return value will be returned to us in each 
case:

- For scalars, `RValue::get(V)` is right.

- We can assert for now that there are no target builtins returning a complex 
type; if we ever need to add this, we'll probably have to break apart an IR 
struct.

- So that just leaves aggregates.  I believe we can't rely on `ReturnValue` 
being non-null on entry to this function; for example, IIRC we'll pass down a 
null slot if the code does an immediate member access into the result, e.g. 
`vld2q(...).val[0]`.  It looks like the custom logic for VLD24 does not do 
anything reasonable in this case: it returns a first-class struct, which the 
rest of IRGen will definitely not expect.  Probably the most reasonable thing 
would be to create a slot if we have an aggregate result and the caller didn't 
give us a slot (out here in `EmitBuiltinExpr`), and then we can just require 
`EmitTargetBuiltinExpr` to return null or something else that makes it clear 
that they've done the right thing with the slot that was passed in.  You can 
create a slot with `CreateMemTemp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72271



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

[PATCH] D71848: Allow the discovery of Android NDK's triple-prefixed binaries.

[Dan Albert via Phabricator via cfe-commits]

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

Just to clarify, this is needed for the triple-prefixed tools, but the 
triple-specific directory worked fine before this patch? If I'm understanding 
that correctly then LGTM, otherwise I'm confused as to why I haven't seen this 
problem before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71848



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

[PATCH] D72281: [Matrix] Add matrix type to Clang (WIP).

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61254 tests passed, 1 failed 
and 736 were skipped.

  failed: Clang.CodeGen/pch-dllexport.cpp

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281



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

[PATCH] D69876: Allow output constraints on "asm goto"

[James Y Knight via Phabricator via cfe-commits]

jyknight accepted this revision.
jyknight added a comment.

LGTM modulo some minor wording.




Comment at: clang/docs/LanguageExtensions.rst:1256-1258
+Clang provides support for the `goto form of GCC's extended
+assembly`_ with output
+constraints.

I find that sentence confusing. Maybe,
"In addition to the functionality provided by , Clang 
also supports output constraints with the goto form."



Comment at: clang/lib/AST/Stmt.cpp:646-648
+  // Labels are placed after "InOut" operands. Adjust accordingly.
+  if (IsLabel)
+N += getNumPlusOperands();

I'm confused about this part. Why isn't the "N" specified in the assembly 
string already the correct value for the labels? Is the ordering we use 
internally and that users use externally not the same? I'm assuming your code 
here is correct, just I'm not understanding, so probably an improved comment 
would be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876



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

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D68101#1806135 , @nickdesaulniers 
wrote:

> In D68101#1802220 , @bd1976llvm 
> wrote:
>
> > Below is the code comment from the new patch explaining the new approach, 
> > please take a look and see if you have any questions/comments:
> >
> >   // If two globals with differing sizes end up in the same mergeable
> >   // section that section can be assigned an incorrect entry size. Normally,
> >   // the assembler avoids this by putting incompatible globals into
> >   // differently named sections. However, globals can be explicitly assigned
> >   // to a section by specifying the section name. In this case, if unique
> >   // section names are available (-unique-section-names in LLVM) then we
> >   // bin compatible globals into different mergeable sections with the same 
> > name.
>
>
> Looks good up to here.
>
> >   // Otherwise, if incompatible globals have been explicitly assigned to 
> > section by a
> >   // fine-grained/per-symbol mechanism (e.g. via  
> > _attribute_((section(“myname” then
> >   // we issue an error and the user can then change the section assignment. 
> > If the
>
> No bueno.  The Linux kernel has code where there are 2D global arrays of 
> different entity sizes explicitly placed in different sections (together). 
> GCC does not error for this, (it doesn't mark the section merge-able), 
> neither should we.
>
> In D68101#1802280 , @rjmccall wrote:
>
> > I laid out a series of three options before:
> >
> > - Emit different object-file sections for different mergeability settings.
> > - Only mark an object-file section as mergeable if all the symbols in it 
> > would have the same mergeability settings.
> > - Stop implicitly using mergeability for "ordinary" sections (i.e. sections 
> > other than the string section).
>
>
> Of the above, I really think #2 is the only complete solution.


Can you explain why you think #1 is not "complete"?  All three seem to 
establish correctness; I can see how giving up on the optimization (#3) is not 
a great solution, but #1 doesn't have that problem and actually preserves it in 
more places.  To be clear, this is just taking advantage of the ability to have 
multiple sections with the same name in an ELF object file; they will still be 
assembled into a single section in the linked image.

My understanding is that changing the flags on an MCSection retroactively is 
pretty counter to the architecture, so I'm not sure how we'd actually achieve 
#2.


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

https://reviews.llvm.org/D68101



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

Kind ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:351
+const std::string &Fixup) {
+  if (Fixup.empty()) {
+return "; cannot be fixed automatically";

Please elide braces. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72284



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

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: llvm/docs/LangRef.rst:1837
+   are present, this overrides ``"denormal-fp-math"``. Not all targets
+   support separately setting the denormal mode per type.
+

Can you document which targets do support the option? What happens if I try to 
use the option on a target where it is not supported?


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

https://reviews.llvm.org/D69878



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4925
+  T.getArch() != llvm::Triple::x86_64) {
+S.Diag(getAttrLoc(AL), diag::err_attribute_unsupported) << AL;
+return;

Why is the target arch also checked in `clang/lib/Driver/ToolChains/Clang.cpp` 
in https://reviews.llvm.org/D7?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4925
+  T.getArch() != llvm::Triple::x86_64) {
+S.Diag(getAttrLoc(AL), diag::err_attribute_unsupported) << AL;
+return;

nickdesaulniers wrote:
> Why is the target arch also checked in 
> `clang/lib/Driver/ToolChains/Clang.cpp` in https://reviews.llvm.org/D7?
Ah, https://reviews.llvm.org/D72221 is checking the `__attribute__` syntax, 
https://reviews.llvm.org/D7 is checking the command line `-f` syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D71848: Allow the discovery of Android NDK's triple-prefixed binaries.

[Brian Ledger via Phabricator via cfe-commits]

brianpl added a comment.

> Just to clarify, this is needed for the triple-prefixed tools, but the 
> triple-specific directory worked fine before this patch? If I'm understanding 
> that correctly then LGTM, otherwise I'm confused as to why I haven't seen 
> this problem before.

Yes, that's right. Without the patch, Clang will overlook the triple-prefixed 
tools, but it will correctly find tools in the triple-specific directory either 
way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71848



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

[PATCH] D72289: [analyzer] Update help text to reflect sarif support

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: aaron.ballman, NoQ.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72289

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2858,7 +2858,7 @@
 def _all_warnings : Flag<["--"], "all-warnings">, Alias;
 def _analyzer_no_default_checks : Flag<["--"], "analyzer-no-default-checks">, 
Flags<[DriverOption]>;
 def _analyzer_output : JoinedOrSeparate<["--"], "analyzer-output">, 
Flags<[DriverOption]>,
-  HelpText<"Static analyzer report output format 
(html|plist|plist-multi-file|plist-html|text).">;
+  HelpText<"Static analyzer report output format 
(html|plist|plist-multi-file|plist-html|sarif|text).">;
 def _analyze : Flag<["--"], "analyze">, Flags<[DriverOption, CoreOption]>,
   HelpText<"Run the static analyzer">;
 def _assemble : Flag<["--"], "assemble">, Alias;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2858,7 +2858,7 @@
 def _all_warnings : Flag<["--"], "all-warnings">, Alias;
 def _analyzer_no_default_checks : Flag<["--"], "analyzer-no-default-checks">, Flags<[DriverOption]>;
 def _analyzer_output : JoinedOrSeparate<["--"], "analyzer-output">, Flags<[DriverOption]>,
-  HelpText<"Static analyzer report output format (html|plist|plist-multi-file|plist-html|text).">;
+  HelpText<"Static analyzer report output format (html|plist|plist-multi-file|plist-html|sarif|text).">;
 def _analyze : Flag<["--"], "analyze">, Flags<[DriverOption, CoreOption]>,
   HelpText<"Run the static analyzer">;
 def _assemble : Flag<["--"], "assemble">, Alias;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:13
+#include "../ClangTidyCheck.h"
+
+#include 

Unnecessary empty line.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:6
+
+Finds usages of ADL (argument-dependent lookup), or potential ADL in the case 
of templates, that are not on the provided whitelist.
+

Please follow 80 character length limit. Same below.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:26
+
+ADL can be surprising, and can lead to `subtle bugs 
`_ without the utmost attention. 
However, it very is useful for lookup of overloaded operators, and for 
customization points within libraries (e.g. `swap` in the C++ standard 
library). As such, this check can be configured to ignore calls to overloaded 
operators as well as other legitimate uses of ADL specified in a whitelist.
+

Please use double back-ticks for swap (or std::swap?).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:35
+
+   If non-zero, ignores calls to overloaded operators using the operator 
syntax (e.g. `a + b`), but not the function call syntax (e.g. `operator+(a, 
b)`). Default is `1`.
+

Indentation. Please use double back-ticks for a + b and operator+(a, b).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:39
+
+   Semicolon-separated list of names that the check ignores. Default is `swap`.

Indentation. Please use double back-ticks for swap (or std::swap?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:826
+  "patchable-function-entry",
+  (Twine(Attr->getSize()) + "," + Twine(Attr->getStart())).str());
   }

ostannard wrote:
> I think using two function attributes here would be better, to avoid needing 
> to parse this again later.
In that case, it would not make sense to have start without a size, and thus 
should be checked for in the verification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D68720: Support -fstack-clash-protection for x86

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:1903
 
+.. option:: -fstack-clash-protection
+

Probably need a -fno-stack-class-protection as well. Looks like gcc has it. 
You'll need to update the handling in the driver to make sure the last polarity 
wins. For cc1 you might be able to support just the positive variant. But try 
to see what we usually do.



Comment at: clang/lib/Basic/Targets/X86.h:152
 
+  const char *getSPRegName() const override { return "rsp"; }
+

What about 32-bit mode where the register name is "esp"?



Comment at: clang/lib/CodeGen/CGStmt.cpp:2254
+CGM.getDiags().Report(S.getAsmLoc(),
+  diag::warn_fe_stack_clash_protection_inline_asm);
+  }

Why is this in the frontend diagnostic list?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2573
+
+  switch (EffectiveTriple.getArch()) {
+  default:

Can we use EffectiveTriple.isX86() that was just introduced yesterday?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2582
+  for (const Arg *A : Args) {
+switch (A->getOption().getID()) {
+default:

Seems like this should just be an if? Or maybe use Args.filtered or args 
getLastArg?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720



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

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Matt Arsenault via Phabricator via cfe-commits]

arsenm marked an inline comment as done.
arsenm added inline comments.



Comment at: llvm/docs/LangRef.rst:1837
+   are present, this overrides ``"denormal-fp-math"``. Not all targets
+   support separately setting the denormal mode per type.
+

andrew.w.kaylor wrote:
> Can you document which targets do support the option? What happens if I try 
> to use the option on a target where it is not supported?
I'm not sure where to document this, or if/how/where to diagnose it. I don't 
think the high level LangRef description is the right place to discuss specific 
target handling.

Currently it won't error or anything. Code checking the denorm mode will see 
the f32 specific mode, even if the target in the end isn't really going to 
respect this.

One problem is this potentially does require coordination with other toolchain 
components. For AMDGPU, the compiler can directly tell the driver what FP mode 
to set on each entry point, but for x86 it requires linking in crtfastmath to 
set the default mode bits. If another target had a similar runtime environment 
requirement, I don't think we can be sure the attribute is correct or not.


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

https://reviews.llvm.org/D69878



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

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 236420.
logan-5 marked 5 inline comments as done.
logan-5 added a comment.

Addressed some formatting stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp
@@ -0,0 +1,75 @@
+// RUN: %check_clang_tidy %s bugprone-unintended-adl %t
+
+namespace aspace {
+struct A {};
+void func(const A &);
+} // namespace aspace
+
+namespace bspace {
+void func(int);
+void test() {
+  aspace::A a;
+  func(5);
+  func(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'aspace::func' through ADL [bugprone-unintended-adl]
+}
+} // namespace bspace
+
+namespace ops {
+struct Stream {
+} stream;
+Stream &operator<<(Stream &s, int) {
+  return s;
+}
+void smooth_operator(Stream);
+} // namespace ops
+
+void test() {
+  ops::stream << 5;
+  operator<<(ops::stream, 5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::operator<<' through ADL [bugprone-unintended-adl]
+  smooth_operator(ops::stream);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ops::smooth_operator' through ADL [bugprone-unintended-adl]
+}
+
+namespace ns {
+struct Swappable {};
+void swap(Swappable &a, Swappable &b); // whitelisted by default
+
+void move(Swappable) {} // non-whitelisted
+template 
+void forward(T) {} // non-whitelisted
+
+struct Swappable2 {};
+} // namespace ns
+
+void move(ns::Swappable2);
+auto ref = [](ns::Swappable2) {};
+
+void test2() {
+  ns::Swappable a, b;
+  using namespace std;
+  swap(a, b);
+  move(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' through ADL [bugprone-unintended-adl]
+  forward(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::forward' through ADL [bugprone-unintended-adl]
+}
+
+template 
+void templateFunction(T t) {
+  swap(t, t);
+
+  move(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'move' may be resolved through ADL [bugprone-unintended-adl]
+
+  ns::move(t);
+  ::move(t);
+
+  ref(t);
+  ::ref(t);
+
+  t << 5;
+  operator<<(t, 5);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unqualified call to 'operator<<' may be resolved through ADL [bugprone-unintended-adl]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - bugprone-unintended-adl
+
+bugprone-unintended-adl
+===
+
+Finds usages of ADL (argument-dependent lookup), or potential ADL in the case 
+of templates, that are not on the provided whitelist.
+
+.. code-block:: c++
+
+  namespace aspace {
+  struct A {};
+  void func(const A &);
+  } // namespace aspace
+  
+  namespace bspace {
+  void func(int);
+  void test() {
+aspace::A a;
+func(5);
+func(a); // calls 'aspace::func' through ADL
+  }
+  } // namespace bspace
+
+(Example respectfully borrowed from `Abseil TotW #49 `_.)
+
+ADL can be surprising, and can lead to `subtle bugs 
+`_ without the utmost attention. 
+However, it very is useful for lookup of overloaded operators, and for 
+customization points within libraries (e.g. ``swap`` in the C++ standard 
+library). As such, this check can be configured to ignore calls to overloaded 
+operators as well as other legitimate uses of ADL specified in a whitelist.
+
+This check does not suggest any fixes.
+
+Options
+---
+
+.. option:: IgnoreOverloadedOperators
+
+   If non-zero, ignores calls to overloaded operators using operator syntax 
+   (e.g. ``a + b``), but not function call syntax (e.g. ``operator+(a, b)``). 
+   Default is `1`.
+
+.. option:: Whitelist
+
+   Semicolon-separated list of names that the check ignores. Default is `swap`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,12 @@
   
   Violating the naming rules above results in undefined behavior.
 
+- New :doc:`bugprone-unintended-adl
+  ` check.
+
+  Finds usages of ADL (argument-

[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:11190
   /// combination, based on their host/device attributes.
-  /// \param Caller function which needs address of \p Callee.
-  ///   nullptr in case of global context.
-  /// \param Callee target function
+  /// \param CallContextDecl the context decl which needs address of \p Callee.
+  ///nullptr in case of global context.

Please capitalize the first word of each of these parameter descriptions, to 
match the style used elsewhere in Clang.



Comment at: clang/include/clang/Sema/Sema.h:11191
+  /// \param CallContextDecl the context decl which needs address of \p Callee.
+  ///nullptr in case of global context.
+  /// \param Callee  target function

"Null" not "nullptr".



Comment at: clang/include/clang/Sema/Sema.h:11198-11206
+  SmallVector CUDANonLocalVariableStack;
+
+  void pushCUDANonLocalVariable(const Decl *D);
+  void popCUDANonLocalVariable(const Decl *D);
+
+  const Decl *getCUDACurrentNonLocalVariable() const {
+return CUDANonLocalVariableStack.empty() ? nullptr

Does this really need to be CUDA-specific?

This is (at least) the third time we've needed this. We currently have a 
`ManglingContextDecl` on `ExpressionEvaluationContextRecord` that tracks the 
non-local variable whose initializer we're parsing. In addition to using this 
as a lambda context declaration, we also (hackily) use it as the context 
declaration for `DiagRuntimeBehavior`. It would seem sensible to use that 
mechanism here too (and rename it to remove any suggestion that this is 
specific to lambdas or mangling).

I think we only currently push `ExpressionEvaluationContext`s for variable 
initializers in C++. That's presumably fine for CUDA's purposes.



Comment at: clang/lib/Parse/ParseDecl.cpp:2336
 
+  Actions.pushCUDANonLocalVariable(ThisDecl);
+

tra wrote:
> @rsmith -- is this sufficient to catch all attempts to call an initializer 
> for a global?
> I wonder if there are other sneaky ways to call an initializer. 
No, this is not sufficient; it's missing (at least) the template instantiation 
case. (The `ExpressionEvaluationContextRecord` mechanism does handle that case 
properly.)

You should also consider what should happen in default arguments (which are 
sometimes parsed before we form a `FunctionDecl` for the function for which 
they are parameters) and default member initializers (which are parsed after we 
know whether the enclosing class has a user-declared default constructor, so 
you could in principle consider the CUDA function kind of the declared 
constructors, I suppose -- but the constructor bodies are not yet available, so 
you can't tell which constructors would actually use the initializers). Both of 
those cases are also tracked by the `ExpressionEvaluationContextRecord` 
mechanism, though you may need to track additional information to process 
default arguments in the same mode as the function for which they are supplied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71227



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

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Logan Smith via Phabricator via cfe-commits]

logan-5 added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:35
+
+   If non-zero, ignores calls to overloaded operators using the operator 
syntax (e.g. `a + b`), but not the function call syntax (e.g. `operator+(a, 
b)`). Default is `1`.
+

Eugene.Zelenko wrote:
> Indentation. Please use double back-ticks for a + b and operator+(a, b).
I'm not sure what you would like me to change about the indentation. This 
follows the same format (three spaces) I've found in the documentation for 
other checks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:39
+
+   Semicolon-separated list of names that the check ignores. Default is `swap`.

Eugene.Zelenko wrote:
> Indentation. Please use double back-ticks for swap (or std::swap?).
Since the 'swap' here is referring to the default value of the option string, 
shouldn't it just be single backticks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282



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

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:39
+
+   Semicolon-separated list of names that the check ignores. Default is `swap`.

logan-5 wrote:
> Eugene.Zelenko wrote:
> > Indentation. Please use double back-ticks for swap (or std::swap?).
> Since the 'swap' here is referring to the default value of the option string, 
> shouldn't it just be single backticks?
It was about swap above, not about option default value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282



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

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D68101#1806213 , @rjmccall wrote:

> In D68101#1806135 , @nickdesaulniers 
> wrote:
>
> > In D68101#1802280 , @rjmccall 
> > wrote:
> >
> > > I laid out a series of three options before:
> > >
> > > - Emit different object-file sections for different mergeability settings.
> > > - Only mark an object-file section as mergeable if all the symbols in it 
> > > would have the same mergeability settings.
> > > - Stop implicitly using mergeability for "ordinary" sections (i.e. 
> > > sections other than the string section).
> >
> >
> > Of the above, I really think #2 is the only complete solution.
>
>
> Can you explain why you think #1 is not "complete"?  All three seem to 
> establish correctness; I can see how giving up on the optimization (#3) is 
> not a great solution, but #1 doesn't have that problem and actually preserves 
> it in more places.  To be clear, this is just taking advantage of the ability 
> to have multiple sections with the same name in an ELF object file; they will 
> still be assembled into a single section in the linked image.
>
> My understanding is that changing the flags on an MCSection retroactively is 
> pretty counter to the architecture, so I'm not sure how we'd actually achieve 
> #2.


Ah, ok, I reread  https://reviews.llvm.org/D72194 and see that it's creating 
non-contiguous sections (option 1), with unique entity sizes.  That should be 
fine. Dissenting opinion retracted.  We should prefer 
https://reviews.llvm.org/D72194 with the commit message updated.


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

https://reviews.llvm.org/D68101



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

[PATCH] D72221: Support function attribute patchable_function_entry

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:686
 
+def PatchableFunctionEntry : InheritableAttr {
+  let Spellings = [GCC<"patchable_function_entry">];

Should this be inheriting from `TargetSpecificAttr` as well given that the 
attribute is only supported on some architectures?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4923
+  const llvm::Triple &T = S.Context.getTargetInfo().getTriple();
+  if (!T.isAArch64() && T.getArch() != llvm::Triple::x86 &&
+  T.getArch() != llvm::Triple::x86_64) {

If you make the attribute target-specific, this code can be removed.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4930
+  uint32_t Size = 0, Start = 0;
+  if (AL.getNumArgs()) {
+if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Size, 0, true))

Should not be a need to check this -- it will always have at least one argument 
because the common attribute handling code will diagnose otherwise given the 
subject list.



Comment at: clang/test/CodeGen/patchable-function-entry.c:20
+// CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0,0"
+// CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2,0"

Can you also add a test verifying that this attribute works with redeclarations 
as well? Something like:
```
[[gnu::patchable_function_entry(12, 4)]] void func(void);

void func(void) { // Definition should have the noop sled
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D69878: Consoldiate internal denormal flushing controls

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: llvm/docs/LangRef.rst:1837
+   are present, this overrides ``"denormal-fp-math"``. Not all targets
+   support separately setting the denormal mode per type.
+

arsenm wrote:
> andrew.w.kaylor wrote:
> > Can you document which targets do support the option? What happens if I try 
> > to use the option on a target where it is not supported?
> I'm not sure where to document this, or if/how/where to diagnose it. I don't 
> think the high level LangRef description is the right place to discuss 
> specific target handling.
> 
> Currently it won't error or anything. Code checking the denorm mode will see 
> the f32 specific mode, even if the target in the end isn't really going to 
> respect this.
> 
> One problem is this potentially does require coordination with other 
> toolchain components. For AMDGPU, the compiler can directly tell the driver 
> what FP mode to set on each entry point, but for x86 it requires linking in 
> crtfastmath to set the default mode bits. If another target had a similar 
> runtime environment requirement, I don't think we can be sure the attribute 
> is correct or not.
There is precedent for describing target-specific behavior in LangRef. It just 
doesn't seem useful to say that not all targets support the attribute without 
saying which ones do. We should also say what is expected if a target doesn't 
support the attribute. It seems reasonable for the function attribute to be 
silently ignored.

> One problem is this potentially does require coordination with other 
> toolchain components. For AMDGPU, the compiler can directly tell the driver 
> what FP mode to set on each entry point, but for x86 it requires linking in 
> crtfastmath to set the default mode bits.

This is a point I'm interested in. I don't like the current crtfastmath.o 
handling. It feels almost accidental when FTZ works as expected. My 
understanding is we link crtfastmath.o if we find it but if not everything just 
goes about its business. The Intel compiler injects code into main() to 
explicitly set the FTZ/DAZ control modes. That obviously has problems too, but 
it's at least consistent and predictable. As I understand it, crtfastmath.o 
sets these modes from a static initializer, but I'm not sure anything is done 
to determine the order of that initializer relative to others.

How does the compiler identify entry points for AMDGPU? And does it emit code 
to set FTZ based on the function attribute here?


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

https://reviews.llvm.org/D69878



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

[PATCH] D72233: Add a new AST matcher 'optionally'.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Just to make sure I understand the purpose -- the goal here is to optionally 
match one or more inner matchers without failing even if none of the inner 
matchers match anything, and this is a different use case than `anyOf()` 
because that would fail when none of the inner matchers match?




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2538
+/// Usable as: Any Matcher
+extern const internal::VariadicOperatorMatcherFunc<
+1, std::numeric_limits::max()>

This needs to be registered in the dynamic matcher registry (Registry.cpp) as 
well. Also, you should regenerate the documentation by running 
`clang\docs\tools\dump_ast_matchers.py`.

I think this matcher could use some example uses in the documentation as well.



Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:362
+BoundNodesTreeBuilder BuilderInner(*Builder);
+if (InnerMatcher.matches(DynNode, Finder, &BuilderInner)) {
+  Result.addMatch(BuilderInner);

You can elide the braces here.


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

https://reviews.llvm.org/D72233



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

[PATCH] D72289: [analyzer] Update help text to reflect sarif support

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM, thank you for this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72289



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

[PATCH] D70689: [analyzer] Fix SARIF column locations

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM! I think we've had sufficient time for other reviewers to lodge concerns 
and we can deal with any other issues post-commit. Do you need me to commit on 
your behalf?




Comment at: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:157
+  const MemoryBuffer *Buf = SM.getBuffer(LocInfo.first);
+  assert(Buf && "got a NULL buffer for the location's file");
+  assert(Buf->getBufferSize() >= (LocInfo.second + TokenLen) &&

jranieri-grammatech wrote:
> aaron.ballman wrote:
> > I don't think this API is able to return a null buffer, but it can return 
> > an invalid buffer under legitimate circumstances, I believe. For instance, 
> > if the source location is in the scratch space used for things like macros 
> > defined on the command line with `-D`. I think you should pass the optional 
> > `invalid` parameter to the call to `getBuffer()` and then do no adjustments 
> > if the buffer is invalid. WDYT?
> Good catch that this can't return NULL.
> 
> I tested out defining things on the command line, but this code deals with 
> expansion locations and wasn't affected by it. Unless you can think of a way 
> to reproduce it, I think I'd rather pass in the `invalid` argument and assert 
> that it isn't invalid.
> I think I'd rather pass in the invalid argument and assert that it isn't 
> invalid.

That's fine by me -- I can't think of another way to reproduce it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70689



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

[PATCH] D70926: [clang-format] Add option for not breaking line before ObjC params

[Kanglei Fang via Phabricator via cfe-commits]

ghvg1313 added a comment.

In D70926#1790848 , @MyDeveloperDay 
wrote:

> Thanks for the patch.


Happy new year!
Could you help landing this diff? I don't know exactly what to do from here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70926



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

[PATCH] D72121: [clang-tidy] Fix readability-identifier-naming missing member variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp:137
+}
+}; // namespace CtorInits

I think this is one more test case to add: base class specifiers. e.g.,
```
struct foo_bar {}; // Should be renamed

struct Baz : foo_bar {}; // Should rename foo_bar
```


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

https://reviews.llvm.org/D72121



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

[PATCH] D72053: [RFC] Handling implementation limits

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/AST/Decl.h:903
 /// declared.
-unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 

These bitfields are assumed to fit into an `unsigned`, and this can no longer 
be verified locally (changes to the `.td` file can break this assumption). 
Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= sizeof(unsigned), 
"...")` next to the `union` below (and likewise for `NonParmVarDeclBitfields`).



Comment at: clang/include/clang/AST/Decl.h:903
 /// declared.
-unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 

rsmith wrote:
> These bitfields are assumed to fit into an `unsigned`, and this can no longer 
> be verified locally (changes to the `.td` file can break this assumption). 
> Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= sizeof(unsigned), 
> "...")` next to the `union` below (and likewise for 
> `NonParmVarDeclBitfields`).
I think `IQ` is too terse. We don't need a really short name for this; how 
about `ImpLimits::` or similar?



Comment at: clang/include/clang/Basic/ImplementationQuantities.td:107-111
+  bit isBitLimit = 0;
+  bit isValueLimit = 0;
+  bit isCompilerFlagLimit = 0;
+  bit isDescriptionLimit = 0;
+  bit hasRemark = 0;

Switching between upper- and lower-case names for these fields seems 
surprising. Please pick a consistent capitalization scheme.



Comment at: clang/lib/CodeGen/CGRecordLayout.h:72
   /// The total size of the bit-field, in bits.
-  unsigned Size : 15;
+  unsigned Size : IQ::BitFieldBits;
 

These three bitfields no longer obviously fit in 32 bits. Please keep the 
hard-coded 15 here and add an adjacent `static_assert(IQ::BitFieldBits <= 15, 
"...")` instead, or use some similar strategy so that the bit-field allocation 
and layout remains locally obvious.


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

https://reviews.llvm.org/D72053



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

[PATCH] D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I guess all the tests run? 
I think this LGTM after the nits are adressed.




Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:203
+  Decl = Ref.getDecl();
+}
+

What about the else? If it is unreachable it should have a 
`llvm_unreachable("Reason");`, if not, i think a little fallthrough comment 
wouldn't hurt.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:259
+
+// Fix type aliases in value declarations
+if (const auto *Value = Result.Nodes.getNodeAs("decl")) {

Please add punctuation here and in the comments below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72284



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

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43
+  Whitelist(
+  utils::options::parseStringList(Options.get("Whitelist", "swap"))) {}
+

do you mean `std::swap`? If you it should be fully qualified.
Doesn't `std::error_code` rely on adl, too? I think `std::cout <<` and other 
streams of the STL rely on it too, and probably many more code-constructs that 
are commonly used.

That means, the list should be extended to at least all standard-library 
facilities that basically required ADL to work. And then we need data on 
different code bases (e.g. LLVM is a good start) how much noise gets generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282



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

[clang] b73fea6 - [NFC] Test commit, whitespace change

[via cfe-commits]

Author: stevewan
Date: 2020-01-06T16:24:27-05:00
New Revision: b73fea6a7cfd87fe07b9c05ba153042198b5d873

URL: 
https://github.com/llvm/llvm-project/commit/b73fea6a7cfd87fe07b9c05ba153042198b5d873
DIFF: 
https://github.com/llvm/llvm-project/commit/b73fea6a7cfd87fe07b9c05ba153042198b5d873.diff

LOG: [NFC] Test commit, whitespace change

As per the Developer Policy, upon obtaining commit access.

Added: 


Modified: 
clang/lib/Driver/ToolChains/AIX.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/AIX.cpp 
b/clang/lib/Driver/ToolChains/AIX.cpp
index 6fbff61f7656..e8d2a14f960f 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -6,6 +6,7 @@
 //
 
//===--===//
 
+
 #include "AIX.h"
 #include "Arch/PPC.h"
 #include "CommonArgs.h"



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

[clang] 02f694b - [NFC] Test commit, revert whitespace change

[via cfe-commits]

Author: stevewan
Date: 2020-01-06T16:28:13-05:00
New Revision: 02f694b69a8b30db7b5d43670da5ab3b9f31bb81

URL: 
https://github.com/llvm/llvm-project/commit/02f694b69a8b30db7b5d43670da5ab3b9f31bb81
DIFF: 
https://github.com/llvm/llvm-project/commit/02f694b69a8b30db7b5d43670da5ab3b9f31bb81.diff

LOG:  [NFC] Test commit, revert whitespace change

As per the Developer Policy, upon obtaining commit access.

Added: 


Modified: 
clang/lib/Driver/ToolChains/AIX.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/AIX.cpp 
b/clang/lib/Driver/ToolChains/AIX.cpp
index e8d2a14f960f..6fbff61f7656 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -6,7 +6,6 @@
 //
 
//===--===//
 
-
 #include "AIX.h"
 #include "Arch/PPC.h"
 #include "CommonArgs.h"



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

[PATCH] D72097: [LifetimeAnalysis] Do not forbid void deref type in gsl::Pointer/gsl::Owner annotations

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


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

https://reviews.llvm.org/D72097



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

[clang] 7b518dc - [OPENMP50]Support lastprivate conditional updates in inc/dec unary ops.

[Alexey Bataev via cfe-commits]

Author: Alexey Bataev
Date: 2020-01-06T16:37:01-05:00
New Revision: 7b518dcb291e740c3e957d93c2b4046bc8a97f00

URL: 
https://github.com/llvm/llvm-project/commit/7b518dcb291e740c3e957d93c2b4046bc8a97f00
DIFF: 
https://github.com/llvm/llvm-project/commit/7b518dcb291e740c3e957d93c2b4046bc8a97f00.diff

LOG: [OPENMP50]Support lastprivate conditional updates in inc/dec unary ops.

Added support for checking of updates of variables used in unary
pre(pos) inc/dec expressions.

Added: 


Modified: 
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CGOpenMPRuntime.h
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/test/OpenMP/for_lastprivate_codegen.cpp
clang/test/OpenMP/sections_lastprivate_codegen.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index e43ed5030bc2..c1a0d5639d12 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1011,6 +1011,9 @@ EmitComplexPrePostIncDec(const UnaryOperator *E, LValue 
LV,
 
   // Store the updated result through the lvalue.
   EmitStoreOfComplex(IncVal, LV, /*init*/ false);
+  if (getLangOpts().OpenMP)
+CGM.getOpenMPRuntime().checkAndEmitLastprivateConditional(*this,
+  E->getSubExpr());
 
   // If this is a postinc, return the value read from memory, otherwise use the
   // updated value.

diff  --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index d759d3682cef..4adaca8ae571 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2356,10 +2356,29 @@ llvm::Value 
*ScalarExprEmitter::EmitIncDecConsiderOverflowBehavior(
   llvm_unreachable("Unknown SignedOverflowBehaviorTy");
 }
 
+namespace {
+/// Handles check and update for lastprivate conditional variables.
+class OMPLastprivateConditionalUpdateRAII {
+private:
+  CodeGenFunction &CGF;
+  const UnaryOperator *E;
+
+public:
+  OMPLastprivateConditionalUpdateRAII(CodeGenFunction &CGF,
+  const UnaryOperator *E)
+  : CGF(CGF), E(E) {}
+  ~OMPLastprivateConditionalUpdateRAII() {
+if (CGF.getLangOpts().OpenMP)
+  CGF.CGM.getOpenMPRuntime().checkAndEmitLastprivateConditional(
+  CGF, E->getSubExpr());
+  }
+};
+} // namespace
+
 llvm::Value *
 ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
bool isInc, bool isPre) {
-
+  OMPLastprivateConditionalUpdateRAII OMPRegion(CGF, E);
   QualType type = E->getSubExpr()->getType();
   llvm::PHINode *atomicPHI = nullptr;
   llvm::Value *value;

diff  --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp 
b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 735cacf0b7d1..b3b16befeea4 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -11447,14 +11447,6 @@ 
CGOpenMPRuntime::LastprivateConditionalRAII::LastprivateConditionalRAII(
   OS << "$pl_cond_" << ID.getDevice() << "_" << ID.getFile() << "_"
  << PLoc.getLine() << "_" << PLoc.getColumn() << "$iv";
   Data.IVName = OS.str();
-
-  // Global loop counter. Required to handle inner parallel-for regions.
-  // global_iv = &iv;
-  QualType PtrIVTy = CGM.getContext().getPointerType(IVLVal.getType());
-  Address GlobIVAddr = CGM.getOpenMPRuntime().getAddrOfArtificialThreadPrivate(
-  CGF, PtrIVTy, Data.IVName);
-  LValue GlobIVLVal = CGF.MakeAddrLValue(GlobIVAddr, PtrIVTy);
-  CGF.EmitStoreOfScalar(IVLVal.getPointer(CGF), GlobIVLVal);
 }
 
 CGOpenMPRuntime::LastprivateConditionalRAII::~LastprivateConditionalRAII() {
@@ -11463,6 +11455,27 @@ 
CGOpenMPRuntime::LastprivateConditionalRAII::~LastprivateConditionalRAII() {
   CGM.getOpenMPRuntime().LastprivateConditionalStack.pop_back();
 }
 
+void CGOpenMPRuntime::initLastprivateConditionalCounter(
+CodeGenFunction &CGF, const OMPExecutableDirective &S) {
+  if (CGM.getLangOpts().OpenMPSimd ||
+  !llvm::any_of(S.getClausesOfKind(),
+[](const OMPLastprivateClause *C) {
+  return C->getKind() == OMPC_LASTPRIVATE_conditional;
+}))
+return;
+  const CGOpenMPRuntime::LastprivateConditionalData &Data =
+  LastprivateConditionalStack.back();
+  if (Data.UseOriginalIV)
+return;
+  // Global loop counter. Required to handle inner parallel-for regions.
+  // global_iv = iv;
+  Address GlobIVAddr = CGM.getOpenMPRuntime().getAddrOfArtificialThreadPrivate(
+  CGF, Data.IVLVal.getType(), Data.IVName);
+  LValue GlobIVLVal = CGF.MakeAddrLValue(GlobIVAddr, Data.IVLVal.getType());
+  llvm::Value *IVVal = CGF.EmitLoadOfScalar(Data.IVLVal, S.getBeginLoc());
+  CGF.EmitStoreOfScalar(IVVal, GlobIVLVal);
+}
+
 namespace {
 /// Checks if the lastprivate conditional variable is referenced in LHS.
 class

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D71714#1795313 , @ilya wrote:

> While I agree with the general notion about the flaws with the current 
> implementation, I feel that the more major refactoring proposed in this 
> review is out of the scope of this minor fix.


I'm OK with making the minor fix here and deferring the larger rework.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13384
   case Stmt::MemberExprClass: {
 expr = cast(expr)->getBase();
 break;

ilya wrote:
> rsmith wrote:
> > Hmm, don't we need to do different things for dot and arrow in this case?
> There are several test cases for an out of bounds access on an array member 
> using dot and arrow operators in array-bounds.cpp. Do you have a specific 
> test case for which you think the code is broken?
> There are several test cases for an out of bounds access on an array member 
> using dot and arrow operators in array-bounds.cpp. Do you have a specific 
> test case for which you think the code is broken?

Sure. There's a false negative for this:

```
struct A { int n; };
A *a[4];
int *n = &a[4]->n;
```

... because we incorrectly visit the left-hand side of the `->` with 
`AllowOnePastEnd == 1`. The left-hand side of `->` is subject to 
lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of the 
context in which the `->` appears.



Comment at: clang/test/SemaCXX/array-bounds.cpp:331
+  Base baseArr[2]; // expected-note {{array 'baseArr' declared here}}
+  Derived *d1 = dynamic_cast(&baseArr[2]); // no warning for 
one-past-end element's address retrieval
+  Derived &d2 = dynamic_cast(baseArr[2]); // expected-warning 
{{array index 2 is past the end of the array (which contains 2 elements)}}

This case should warn; `dynamic_cast` will access the object's vptr. Please at 
least add a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 236449.
logan-5 marked 3 inline comments as done.
logan-5 added a comment.

Addressed some nits. If the rest looks good, I'll need someone with commit 
access to help me wrap this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72284

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h

Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
@@ -0,0 +1,145 @@
+//===--- RenamderClangTidyCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_RENAMERCLANGTIDYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_RENAMERCLANGTIDYCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include 
+#include 
+
+namespace clang {
+
+class MacroInfo;
+
+namespace tidy {
+
+/// Base class for clang-tidy checks that want to flag declarations and/or
+/// macros for renaming based on customizable criteria.
+class RenamerClangTidyCheck : public ClangTidyCheck {
+public:
+  RenamerClangTidyCheck(StringRef CheckName, ClangTidyContext *Context);
+  ~RenamerClangTidyCheck();
+
+  /// Derived classes should not implement any matching logic themselves; this
+  /// class will do the matching and call the derived class'
+  /// GetDeclFailureInfo() and GetMacroFailureInfo() for determining whether a
+  /// given identifier passes or fails the check.
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override final;
+  void
+  check(const ast_matchers::MatchFinder::MatchResult &Result) override final;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override final;
+  void onEndOfTranslationUnit() override final;
+
+  /// This enum will be used in %select of the diagnostic message.
+  /// Each value below IgnoreFailureThreshold should have an error message.
+  enum class ShouldFixStatus {
+ShouldFix,
+ConflictsWithKeyword, /// The fixup will conflict with a language keyword,
+  /// so we can't fix it automatically.
+ConflictsWithMacroDefinition, /// The fixup will conflict with a macro
+  /// definition, so we can't fix it
+  /// automatically.
+
+/// Values pass this threshold will be ignored completely
+/// i.e no message, no fixup.
+IgnoreFailureThreshold,
+
+InsideMacro, /// If the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+  };
+
+  /// Information describing a failed check
+  struct FailureInfo {
+std::string KindName; // Tag or misc info to be used as derived classes need
+std::string Fixup;// The name that will be proposed as a fix-it hint
+  };
+
+  /// Holds an identifier name check failure, tracking the kind of the
+  /// identifier, its possible fixup and the starting locations of all the
+  /// identifier usages.
+  struct NamingCheckFailure {
+FailureInfo Info;
+
+/// Whether the failure should be fixed or not.
+///
+/// e.g.: if the identifier was used or declared within a macro we won't
+/// offer a fixup for safety reasons.
+bool ShouldFix() const {
+  return FixStatus == ShouldFixStatus::ShouldFix && !Info.Fixup.empty();
+}
+
+bool ShouldNotify() const {
+  return FixStatus < ShouldFixStatus::IgnoreFailureThreshold;
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
+
+/// A set of all the identifier usages starting SourceLocation, in
+/// their encoded form.
+llvm::DenseSet RawUsageLocs;
+
+NamingCheckFailure() = default;
+  };
+
+  typedef std::pair NamingCheckId;
+
+  typedef llvm::DenseMap
+  NamingCheckFailureMap;
+
+  /// Check Macros for style violations.
+  void checkMacro(SourceManager &sourceMgr, const Token &MacroNameTok,
+  const MacroInfo *MI);
+
+  /// Add a usage of a macro if it already has a violation.
+  void expandMacro(const Token &MacroNameTok, const MacroInfo *MI);
+
+protected:
+  /// Overridden by derived classes, returns information about if and how a Decl
+  /// 

[PATCH] D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:272
+  if (const auto *Typedef =
+  Value->getReturnType().getTypePtr()->getAs()) {
+addUsage(NamingCheckFailures, Typedef->getDecl(),

Please elide braces.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:280
+ .getTypePtr()
+ ->getAs()) {
+  addUsage(NamingCheckFailures, Typedef->getDecl(),

Please elide braces.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:47
+ShouldFix,
+ConflictsWithKeyword, /// The fixup will conflict with a language keyword,
+  /// so we can't fix it automatically.

It'll be reasonable to place all comments consistently: before definition or 
after.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:94
+
+  typedef std::pair NamingCheckId;
+

Please use using. Same below. See modernize-use-using.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72284



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

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked an inline comment as done.
logan-5 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43
+  Whitelist(
+  utils::options::parseStringList(Options.get("Whitelist", "swap"))) {}
+

JonasToth wrote:
> do you mean `std::swap`? If you it should be fully qualified.
> Doesn't `std::error_code` rely on adl, too? I think `std::cout <<` and other 
> streams of the STL rely on it too, and probably many more code-constructs 
> that are commonly used.
> 
> That means, the list should be extended to at least all standard-library 
> facilities that basically required ADL to work. And then we need data on 
> different code bases (e.g. LLVM is a good start) how much noise gets 
> generated.
I distinctly //don't// mean `std::swap` -- I want to whitelist any unqualified 
function call spelled simply `swap`.

Overloaded operators are the poster child for ADL's usefulness, so that's why 
this check has a special default-on `IgnoreOverloadedOperators` option. That 
whitelists a whole ton of legitimate stuff including `std::cout << x` and 
friends.

I don't see a ton of discussion online about `error_code`/`make_error_code` and 
ADL being very much intertwined. I'm not particularly familiar with those 
constructs myself though, and I could just be out of the loop. I do see a fair 
number of unqualified calls to `make_error_code` within LLVM, though most of 
those resolve to `llvm::make_error_code`, the documentation for which says it 
exists because `std::make_error_code` can't be reliably/portably used with ADL. 
That makes me think `make_error_code` would belong in LLVM's project-specific 
whitelist configuration, not the check's default.

Speaking of which, I did run this check over LLVM while developing and found it 
not particularly noisy as written. That is, it generated a fair number of 
warnings, but only on constructs that, when examined closely, //were// a little 
suspicious or non-obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282



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

[clang] f5329bf - [Diagnostic] make Wmisleading-indendation not warn about labels

[via cfe-commits]

Author: Tyker
Date: 2020-01-06T23:22:27+01:00
New Revision: f5329bfc76bb6fc30a589e8238aabc005c52e5d6

URL: 
https://github.com/llvm/llvm-project/commit/f5329bfc76bb6fc30a589e8238aabc005c52e5d6
DIFF: 
https://github.com/llvm/llvm-project/commit/f5329bfc76bb6fc30a589e8238aabc005c52e5d6.diff

LOG: [Diagnostic] make Wmisleading-indendation not warn about labels

Reviewers: aaron.ballman, xbolva00

Reviewed By: aaron.ballman

Subscribers: nickdesaulniers, nathanchance

Differential Revision: https://reviews.llvm.org/D72202

Added: 


Modified: 
clang/lib/Parse/ParseStmt.cpp
clang/test/Parser/warn-misleading-indentation.cpp

Removed: 




diff  --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index b79654015653..0339328ca513 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -1272,10 +1272,12 @@ struct MisleadingIndentationChecker {
 
 if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
 ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
- !Tok.isAtStartOfLine()) && SM.getPresumedLineNumber(StmtLoc) !=
-  SM.getPresumedLineNumber(Tok.getLocation())) {
-  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
-  << Kind;
+ !Tok.isAtStartOfLine()) &&
+SM.getPresumedLineNumber(StmtLoc) !=
+SM.getPresumedLineNumber(Tok.getLocation()) &&
+(Tok.isNot(tok::identifier) ||
+ P.getPreprocessor().LookAhead(0).isNot(tok::colon))) {
+  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) << Kind;
   P.Diag(StmtLoc, diag::note_previous_statement);
 }
   }

diff  --git a/clang/test/Parser/warn-misleading-indentation.cpp 
b/clang/test/Parser/warn-misleading-indentation.cpp
index 4b9d45b19ca6..8339f21099c3 100644
--- a/clang/test/Parser/warn-misleading-indentation.cpp
+++ b/clang/test/Parser/warn-misleading-indentation.cpp
@@ -305,3 +305,10 @@ int main(int argc, char* argv[]) {
 fail:;
 }
 
+void f_label(int b) {
+  if (b)
+return;
+a:
+  return;
+  goto a;
+}



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

[PATCH] D72202: [Diagnostic] make Wmisleading-indendation not warn about labels

[Tyker via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf5329bfc76bb: [Diagnostic] make Wmisleading-indendation not 
warn about labels (authored by Tyker).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72202

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Parser/warn-misleading-indentation.cpp


Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- clang/test/Parser/warn-misleading-indentation.cpp
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -305,3 +305,10 @@
 fail:;
 }
 
+void f_label(int b) {
+  if (b)
+return;
+a:
+  return;
+  goto a;
+}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1272,10 +1272,12 @@
 
 if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
 ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
- !Tok.isAtStartOfLine()) && SM.getPresumedLineNumber(StmtLoc) !=
-  SM.getPresumedLineNumber(Tok.getLocation())) {
-  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
-  << Kind;
+ !Tok.isAtStartOfLine()) &&
+SM.getPresumedLineNumber(StmtLoc) !=
+SM.getPresumedLineNumber(Tok.getLocation()) &&
+(Tok.isNot(tok::identifier) ||
+ P.getPreprocessor().LookAhead(0).isNot(tok::colon))) {
+  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) << Kind;
   P.Diag(StmtLoc, diag::note_previous_statement);
 }
   }


Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- clang/test/Parser/warn-misleading-indentation.cpp
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -305,3 +305,10 @@
 fail:;
 }
 
+void f_label(int b) {
+  if (b)
+return;
+a:
+  return;
+  goto a;
+}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1272,10 +1272,12 @@
 
 if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
 ((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
- !Tok.isAtStartOfLine()) && SM.getPresumedLineNumber(StmtLoc) !=
-  SM.getPresumedLineNumber(Tok.getLocation())) {
-  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
-  << Kind;
+ !Tok.isAtStartOfLine()) &&
+SM.getPresumedLineNumber(StmtLoc) !=
+SM.getPresumedLineNumber(Tok.getLocation()) &&
+(Tok.isNot(tok::identifier) ||
+ P.getPreprocessor().LookAhead(0).isNot(tok::colon))) {
+  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) << Kind;
   P.Diag(StmtLoc, diag::note_previous_statement);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72053: [RFC] Handling implementation limits

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 6 inline comments as done.
Mordante added a comment.

Thanks for the feedback!




Comment at: clang/include/clang/AST/Decl.h:903
 /// declared.
-unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 

rsmith wrote:
> rsmith wrote:
> > These bitfields are assumed to fit into an `unsigned`, and this can no 
> > longer be verified locally (changes to the `.td` file can break this 
> > assumption). Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= 
> > sizeof(unsigned), "...")` next to the `union` below (and likewise for 
> > `NonParmVarDeclBitfields`).
> I think `IQ` is too terse. We don't need a really short name for this; how 
> about `ImpLimits::` or similar?
I feared ImpLimits would be considered too verbose, but I'll change it in the 
next revision.



Comment at: clang/include/clang/AST/Decl.h:903
 /// declared.
-unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 

Mordante wrote:
> rsmith wrote:
> > rsmith wrote:
> > > These bitfields are assumed to fit into an `unsigned`, and this can no 
> > > longer be verified locally (changes to the `.td` file can break this 
> > > assumption). Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= 
> > > sizeof(unsigned), "...")` next to the `union` below (and likewise for 
> > > `NonParmVarDeclBitfields`).
> > I think `IQ` is too terse. We don't need a really short name for this; how 
> > about `ImpLimits::` or similar?
> I feared ImpLimits would be considered too verbose, but I'll change it in the 
> next revision.
I'm not too fond to use the `static_assert(sizeof(ParamVarDeclBitFields <= 
sizeof(unsigned), "...");` here and use a hardcoded value and a  
`static_assert` in other places. If it's important to see the size of bit-field 
directly I prefer the style you suggested later: "Please keep the hard-coded 15 
here and add an adjacent `static_assert(IQ::BitFieldBits <= 15, "...")` 
instead, or use some similar strategy so that the bit-field allocation and 
layout remains locally obvious."

I'll give it some more thoughts before updating the proposal.



Comment at: clang/include/clang/Basic/ImplementationQuantities.td:107-111
+  bit isBitLimit = 0;
+  bit isValueLimit = 0;
+  bit isCompilerFlagLimit = 0;
+  bit isDescriptionLimit = 0;
+  bit hasRemark = 0;

rsmith wrote:
> Switching between upper- and lower-case names for these fields seems 
> surprising. Please pick a consistent capitalization scheme.
This style is used in other .td files. But I'll switch to capitalize the is/has 
in the next revision. 


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

https://reviews.llvm.org/D72053



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

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Fady Ghanim via Phabricator via cfe-commits]

fghanim created this revision.
fghanim added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, guansong, hiraditya.
Herald added projects: clang, LLVM.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72304

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -613,4 +613,180 @@
   }
 }
 
+TEST_F(OpenMPIRBuilderTest, MasterDirective) {
+	using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+	OpenMPIRBuilder OMPBuilder(*M);
+	OMPBuilder.initialize();
+	F->setName("func");
+	IRBuilder<> Builder(BB);
+
+	OpenMPIRBuilder::LocationDescription Loc( { Builder.saveIP(), DL });
+
+	AllocaInst *PrivAI = nullptr;
+
+	BasicBlock *EntryBB = nullptr;
+	BasicBlock *FinalBB = nullptr;
+	BasicBlock *ExitBB = nullptr;
+	BasicBlock *ThenBB = nullptr;
+
+	auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+			BasicBlock &FiniBB) {
+		if (AllocaIP.isSet())
+			Builder.restoreIP(AllocaIP);
+		else
+			Builder.SetInsertPoint(&*(F->getEntryBlock().getFirstInsertionPt()));
+		PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+		Builder.CreateStore(F->arg_begin(), PrivAI);
+
+		llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+		llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+		EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+
+		Builder.restoreIP(CodeGenIP);
+
+		//collect some info for checks later
+		FinalBB = &FiniBB;
+		ExitBB = FiniBB.getUniqueSuccessor();
+		ThenBB = Builder.GetInsertBlock();
+		EntryBB = ThenBB->getUniquePredecessor();
+
+		//simple instructions for body
+		Value *PrivLoad = Builder.CreateLoad(PrivAI, "local.use");
+		Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+	};
+
+	auto FiniCB = [&](InsertPointTy IP) {
+		BasicBlock *IPBB = IP.getBlock();
+		EXPECT_NE(IPBB->end(), IP.getPoint());
+	};
+
+	Builder.restoreIP(OMPBuilder.CreateMaster(Builder, BodyGenCB, FiniCB));
+	Value *EntryBBTI = EntryBB->getTerminator();
+	EXPECT_NE(EntryBBTI, nullptr);
+	EXPECT_TRUE(isa(EntryBBTI));
+	BranchInst *EntryBr = cast(EntryBB->getTerminator());
+	EXPECT_TRUE(EntryBr->isConditional());
+	EXPECT_EQ(EntryBr->getSuccessor(0), ThenBB);
+	EXPECT_EQ(ThenBB->getUniqueSuccessor(), FinalBB);
+	EXPECT_EQ(FinalBB->getUniqueSuccessor(), ExitBB);
+	EXPECT_EQ(EntryBr->getSuccessor(1), ExitBB);
+
+	CmpInst *CondInst = cast(EntryBr->getCondition());
+	EXPECT_TRUE(isa(CondInst->getOperand(0)));
+
+	CallInst *MasterEntryCI = cast(CondInst->getOperand(0));
+	EXPECT_EQ(MasterEntryCI->getNumArgOperands(), 2U);
+	EXPECT_EQ(MasterEntryCI->getCalledFunction()->getName(), "__kmpc_omp_master");
+	EXPECT_TRUE(isa(MasterEntryCI->getArgOperand(0)));
+
+	CallInst *MasterEndCI = nullptr;
+	for (auto FI = FinalBB->begin(); FI != FinalBB->end(); ++FI) {
+		Instruction *cur = &*FI;
+		if (isa(cur)) {
+			MasterEndCI = cast(cur);
+			if (MasterEndCI->getCalledFunction()->getName()
+	== "__kmpc_omp_end_master")
+break;
+			else
+MasterEndCI = nullptr;
+		}
+	}
+	EXPECT_NE(MasterEndCI, nullptr);
+	EXPECT_EQ(MasterEndCI->getNumArgOperands(), 2U);
+	EXPECT_TRUE(isa(MasterEndCI->getArgOperand(0)));
+	EXPECT_EQ(MasterEndCI->getArgOperand(1), MasterEntryCI->getArgOperand(1));
+}
+
+TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
+	using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+	OpenMPIRBuilder OMPBuilder(*M);
+	OMPBuilder.initialize();
+	F->setName("func");
+	IRBuilder<> Builder(BB);
+
+	OpenMPIRBuilder::LocationDescription Loc( { Builder.saveIP(), DL });
+
+	AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType());
+
+	BasicBlock *EntryBB = nullptr;
+	BasicBlock *FinalBB = nullptr;
+	BasicBlock *ExitBB = nullptr;
+
+	auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+			BasicBlock &FiniBB) {
+		//collect some info for checks later
+		FinalBB = &FiniBB;
+		ExitBB = FiniBB.getUniqueSuccessor();
+		EntryBB = FinalBB->getUniquePredecessor();
+
+		//actual start for bodyCB
+		llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+		llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+		EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+		EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+		//body begin
+		Builder.restoreIP(CodeGenIP);
+		Builder.CreateStore(F->arg_begin(), PrivAI);
+		Value *PrivLoad = Builder.CreateLoad(PrivAI, "local.use");
+		Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+	};
+
+	auto FiniCB = [&](InsertPointTy IP) {
+		BasicBlock *IPBB = IP.getBlock();
+		EXPECT_NE(IPBB->end(), IP.getPoint());
+	};
+
+	Builder.restoreIP(
+			OMPBuilder.CreateCritical(Builder, BodyGenCB, FiniCB, "testCRT",
+	

[PATCH] D72221: Support function attribute patchable_function_entry

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 236468.
MaskRay marked 13 inline comments as done.
MaskRay added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/patchable-function-entry.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/patchable-function-entry-attr.c
  clang/test/Sema/patchable-function-entry-attr.cpp

Index: clang/test/Sema/patchable-function-entry-attr.cpp
===
--- /dev/null
+++ clang/test/Sema/patchable-function-entry-attr.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple i386 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple ppc64le -fsyntax-only -verify %s
+
+// silence-no-diagnostics
+
+// expected-warning@+1 {{unknown attribute 'patchable_function_entry' ignored}}
+[[gnu::patchable_function_entry(0)]] void f();
Index: clang/test/Sema/patchable-function-entry-attr.c
===
--- /dev/null
+++ clang/test/Sema/patchable-function-entry-attr.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify %s
+
+// expected-error@+1 {{'patchable_function_entry' attribute takes at least 1 argument}}
+__attribute__((patchable_function_entry)) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute takes no more than 2 arguments}}
+__attribute__((patchable_function_entry(0, 0, 0))) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute requires a non-negative integral compile time constant expression}}
+__attribute__((patchable_function_entry(-1))) void f();
+
+int i;
+// expected-error@+1 {{'patchable_function_entry' attribute requires parameter 0 to be an integer constant}}
+__attribute__((patchable_function_entry(i))) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute requires integer constant between 0 and 0 inclusive}}
+__attribute__((patchable_function_entry(1, 1))) void f();
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -128,6 +128,7 @@
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: PatchableFunctionEntry (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ReleaseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
Index: clang/test/CodeGen/patchable-function-entry.c
===
--- /dev/null
+++ clang/test/CodeGen/patchable-function-entry.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple aarch64 -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define void @f0() #0
+__attribute__((patchable_function_entry(0))) void f0() {}
+
+// CHECK: define void @f00() #0
+__attribute__((patchable_function_entry(0, 0))) void f00() {}
+
+// CHECK: define void @f2() #1
+__attribute__((patchable_function_entry(2))) void f2() {}
+
+// CHECK: define void @f20() #1
+__attribute__((patchable_function_entry(2, 0))) void f20() {}
+
+// CHECK: define void @f20decl() #1
+__attribute__((patchable_function_entry(2, 0))) void f20decl();
+void f20decl() {}
+
+/// M in patchable_function_entry(N,M) is currently ignored.
+// CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0"
+// CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2"
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4917,6 +4917,25 @@
  XRayLogArgsAttr(S.Context, AL, ArgCount.getSourceIndex()));
 }
 
+static void handlePatchableFunctionEntryAttr(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
+  uint32_t Count = 0, Offset = 0;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Count, 0, true))
+return;
+  if (AL.getNumArgs() == 2) {
+Expr *Arg = AL.getArgAsExpr(1);
+if (!checkUInt32Argument(S, AL, Arg, Offset, 1, true))
+  return;
+if (Offset) {
+  S.Diag(getAttrLoc(AL), diag::err_attribute_argument_out_of_range)
+  << &AL << 0 << 0 << Arg->getBeginLoc();
+  ret

[PATCH] D72221: Support function attribute patchable_function_entry

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:686
 
+def PatchableFunctionEntry : InheritableAttr {
+  let Spellings = [GCC<"patchable_function_entry">];

aaron.ballman wrote:
> Should this be inheriting from `TargetSpecificAttr` as well given that the 
> attribute is only supported on some architectures?
Thanks for the suggestion. Didn't know TargetSpecificAttr.



Comment at: clang/include/clang/Basic/AttrDocs.td:3995
+``__attribute__((patchable_function_entry(N,M)))`` is used to generate M NOPs
+before the function entry and N-M NOPs after the function entry. This 
attributes
+takes precedence over command line option ``-fpatchable-function-entry=N,M``.

ostannard wrote:
> Grammar nits:
> s/attributes/attribute/
> over _the_ command line option
Thanks. (I am a non-native English speaker and any advice will be highly 
appreciated.)

(So, I believe GCC's documentation misses _the_)



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:826
+  "patchable-function-entry",
+  (Twine(Attr->getSize()) + "," + Twine(Attr->getStart())).str());
   }

nickdesaulniers wrote:
> ostannard wrote:
> > I think using two function attributes here would be better, to avoid 
> > needing to parse this again later.
> In that case, it would not make sense to have start without a size, and thus 
> should be checked for in the verification.
Agreed. Updated D72215 (semantics of the IR function attribute 
"patchable-function-entry") as well.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4925
+  T.getArch() != llvm::Triple::x86_64) {
+S.Diag(getAttrLoc(AL), diag::err_attribute_unsupported) << AL;
+return;

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Why is the target arch also checked in 
> > `clang/lib/Driver/ToolChains/Clang.cpp` in https://reviews.llvm.org/D7?
> Ah, https://reviews.llvm.org/D72221 is checking the `__attribute__` syntax, 
> https://reviews.llvm.org/D7 is checking the command line `-f` syntax.
Yes. D72221 is for the Clang function attribute while D7 is for 
`-fpatchable-function-entry=`.





Comment at: clang/test/CodeGen/patchable-function-entry.c:20
+// CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0,0"
+// CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2,0"

aaron.ballman wrote:
> Can you also add a test verifying that this attribute works with 
> redeclarations as well? Something like:
> ```
> [[gnu::patchable_function_entry(12, 4)]] void func(void);
> 
> void func(void) { // Definition should have the noop sled
> }
> ```
```
__attribute__((patchable_function_entry(2,0))) void f20decl();
__attribute__((noinline)) void f20decl() {}
```

Checked this case. I'll delete `__attribute__((noinline))` since it does not 
seem to be clear what it intends to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221



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

[PATCH] D72306: [PowerPC] FreeBSD >= 13 default ABI is ELFv2

[Alfredo Dal'Ava Júnior via Phabricator via cfe-commits]

adalava created this revision.
adalava added reviewers: dim, MaskRay.
adalava added a project: PowerPC.
Herald added subscribers: llvm-commits, cfe-commits, steven.zhang, shchenz, 
jsji, kbarton, hiraditya, krytarowski, arichardson, nemanjai, emaste.
Herald added projects: clang, LLVM.

FreeBSD for PowerPC* transitioned[1][2] from gcc4.2 to LLVM9 on Dec 26, 2019 
(starting from r356111). The PowerPC64 target also transitioned to ELFv2 ABI at 
same time,  so this makes it default for FreeBSD releases >= 13

*Also would like to have this back ported to LLVM 9 on upstream if possible

[1] https://wiki.freebsd.org/powerpc/llvm-elfv2
[2] https://lists.freebsd.org/pipermail/freebsd-ppc/2019-December/011042.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72306

Files:
  clang/lib/Basic/Targets/PPC.h
  llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
  llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll

Index: llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll
===
--- llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll
+++ llvm/test/CodeGen/PowerPC/ppc64-elf-abi.ll
@@ -5,6 +5,30 @@
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
 
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd11 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd12 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd14 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd11 -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd11 -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd12 -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd12 -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13 -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13 -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd14 -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd14 -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd11-elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd11-elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd12-elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd12-elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13-elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd13-elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd14-elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd14-elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
+
+; FreeBSD target triple without OS version number is incorrect, but checking for regressions anyway
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd < %s | FileCheck %s -check-prefix=CHECK-ELFv1
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd < %s | FileCheck %s -check-prefix=CHECK-ELFv1
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd -target-abi elfv1 < %s | FileCheck %s -check-prefix=CHECK-ELFv1
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-freebsd -target-abi elfv2 < %s | FileCheck %s -check-prefix=CHECK-ELFv2
Index: llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
===
--- llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
+++ llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
@@ -199,7 +199,7 @@
  const TargetOptions &Options) {
   if (TT.isOSDarwin())
 report_fatal_error("Darwin is no longer supported for PowerPC");
-  
+
   if (Options.MCOpt

[PATCH] D72247: Add Triple::isX86()

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

@craig.topper Good to go? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72247



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

[PATCH] D72247: Add Triple::isX86()

[Craig Topper via Phabricator via cfe-commits]

craig.topper accepted this revision.
craig.topper added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72247



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