[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Kindly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

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

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

There are a few places (primarily in ADT and Support) that check __cplusplus > 
201402. Do they need to be cleaned up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a reviewer: aaron.ballman.
barannikov88 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.h:63
+  /// Returns Swift ABI info helper for the target.
+  const SwiftABIInfo &getSwiftABIInfo() const { return *SwiftInfo; }
+

aaron.ballman wrote:
> `SwiftInfo` isn't always initialized to a nonnull pointer; should this have 
> an assert to help point out when someone's forgotten to initialize that 
> properly?
Good point, thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 450298.
barannikov88 added a comment.

Assert that Swift ABI info is initialized before accessing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

Files:
  clang/lib/CodeGen/ABIInfo.h
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -38,6 +38,7 @@
 class CallArgList;
 class CodeGenFunction;
 class CGBlockInfo;
+class SwiftABIInfo;
 
 /// TargetCodeGenInfo - This class organizes various target-specific
 /// codegeneration issues, like target-specific attributes, builtins and so
@@ -45,6 +46,12 @@
 class TargetCodeGenInfo {
   std::unique_ptr Info;
 
+protected:
+  // Target hooks supporting Swift calling conventions. The target must
+  // initialize this field if it claims to support these calling conventions
+  // by returning true from TargetInfo::checkCallingConvention for them.
+  std::unique_ptr SwiftInfo;
+
 public:
   TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
@@ -52,6 +59,12 @@
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo &getABIInfo() const { return *Info; }
 
+  /// Returns Swift ABI info helper for the target.
+  const SwiftABIInfo &getSwiftABIInfo() const {
+assert(SwiftInfo && "Swift ABI info has not been initialized");
+return *SwiftInfo;
+  }
+
   /// setTargetAttributes - Provides a convenient hook to handle extra
   /// target-specific attributes for the given global.
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -23,7 +23,6 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
-#include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -117,7 +116,9 @@
   return false;
 }
 
-ABIInfo::~ABIInfo() {}
+ABIInfo::~ABIInfo() = default;
+
+SwiftABIInfo::~SwiftABIInfo() = default;
 
 /// Does the given lowering require more than the given number of
 /// registers when expanded?
@@ -151,12 +152,16 @@
   return (intCount + fpCount > maxAllRegisters);
 }
 
-bool SwiftABIInfo::isLegalVectorTypeForSwift(CharUnits vectorSize,
- llvm::Type *eltTy,
- unsigned numElts) const {
+bool SwiftABIInfo::shouldPassIndirectly(ArrayRef ComponentTys,
+bool AsReturnValue) const {
+  return occupiesMoreThan(CGT, ComponentTys, /*total=*/4);
+}
+
+bool SwiftABIInfo::isLegalVectorType(CharUnits VectorSize, llvm::Type *EltTy,
+ unsigned NumElts) const {
   // The default implementation of this assumes that the target guarantees
   // 128-bit SIMD support but nothing more.
-  return (vectorSize.getQuantity() > 8 && vectorSize.getQuantity() <= 16);
+  return (VectorSize.getQuantity() > 8 && VectorSize.getQuantity() <= 16);
 }
 
 static CGCXXABI::RecordArgABI getRecordArgABI(const RecordType *RT,
@@ -814,7 +819,7 @@
 // This is a very simple ABI that relies a lot on DefaultABIInfo.
 //===--===//
 
-class WebAssemblyABIInfo final : public SwiftABIInfo {
+class WebAssemblyABIInfo final : public ABIInfo {
 public:
   enum ABIKind {
 MVP = 0,
@@ -827,7 +832,7 @@
 
 public:
   explicit WebAssemblyABIInfo(CodeGen::CodeGenTypes &CGT, ABIKind Kind)
-  : SwiftABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
+  : ABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
 
 private:
   ABIArgInfo classifyReturnType(QualType RetTy) const;
@@ -845,22 +850,16 @@
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType Ty) const override;
-
-  bool shouldPassIndirectlyForSwift(ArrayRef scalars,
-bool asReturnValue) const override {
-return occupiesMoreThan(CGT, scalars, /*total*/ 4);
-  }
-
-  bool isSwiftErrorInRegister() const override {
-return false;
-  }
 };
 
 class WebAssemblyTargetCodeGenInfo final : public TargetCodeGenInfo {
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {
+SwiftInfo =
+std::make_unique(CGT, /*SwiftErrorInRegister=*/false);
+  }
 
   void setTargetAttributes(const 

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 450294.
barannikov88 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

Files:
  clang/lib/CodeGen/ABIInfo.h
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -38,6 +38,7 @@
 class CallArgList;
 class CodeGenFunction;
 class CGBlockInfo;
+class SwiftABIInfo;
 
 /// TargetCodeGenInfo - This class organizes various target-specific
 /// codegeneration issues, like target-specific attributes, builtins and so
@@ -45,6 +46,12 @@
 class TargetCodeGenInfo {
   std::unique_ptr Info;
 
+protected:
+  // Target hooks supporting Swift calling conventions. The target must
+  // initialize this field if it claims to support these calling conventions
+  // by returning true from TargetInfo::checkCallingConvention for them.
+  std::unique_ptr SwiftInfo;
+
 public:
   TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
@@ -52,6 +59,9 @@
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo &getABIInfo() const { return *Info; }
 
+  /// Returns Swift ABI info helper for the target.
+  const SwiftABIInfo &getSwiftABIInfo() const { return *SwiftInfo; }
+
   /// setTargetAttributes - Provides a convenient hook to handle extra
   /// target-specific attributes for the given global.
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -23,7 +23,6 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
-#include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -117,7 +116,9 @@
   return false;
 }
 
-ABIInfo::~ABIInfo() {}
+ABIInfo::~ABIInfo() = default;
+
+SwiftABIInfo::~SwiftABIInfo() = default;
 
 /// Does the given lowering require more than the given number of
 /// registers when expanded?
@@ -151,12 +152,16 @@
   return (intCount + fpCount > maxAllRegisters);
 }
 
-bool SwiftABIInfo::isLegalVectorTypeForSwift(CharUnits vectorSize,
- llvm::Type *eltTy,
- unsigned numElts) const {
+bool SwiftABIInfo::shouldPassIndirectly(ArrayRef ComponentTys,
+bool AsReturnValue) const {
+  return occupiesMoreThan(CGT, ComponentTys, /*total=*/4);
+}
+
+bool SwiftABIInfo::isLegalVectorType(CharUnits VectorSize, llvm::Type *EltTy,
+ unsigned NumElts) const {
   // The default implementation of this assumes that the target guarantees
   // 128-bit SIMD support but nothing more.
-  return (vectorSize.getQuantity() > 8 && vectorSize.getQuantity() <= 16);
+  return (VectorSize.getQuantity() > 8 && VectorSize.getQuantity() <= 16);
 }
 
 static CGCXXABI::RecordArgABI getRecordArgABI(const RecordType *RT,
@@ -814,7 +819,7 @@
 // This is a very simple ABI that relies a lot on DefaultABIInfo.
 //===--===//
 
-class WebAssemblyABIInfo final : public SwiftABIInfo {
+class WebAssemblyABIInfo final : public ABIInfo {
 public:
   enum ABIKind {
 MVP = 0,
@@ -827,7 +832,7 @@
 
 public:
   explicit WebAssemblyABIInfo(CodeGen::CodeGenTypes &CGT, ABIKind Kind)
-  : SwiftABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
+  : ABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
 
 private:
   ABIArgInfo classifyReturnType(QualType RetTy) const;
@@ -845,22 +850,16 @@
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType Ty) const override;
-
-  bool shouldPassIndirectlyForSwift(ArrayRef scalars,
-bool asReturnValue) const override {
-return occupiesMoreThan(CGT, scalars, /*total*/ 4);
-  }
-
-  bool isSwiftErrorInRegister() const override {
-return false;
-  }
 };
 
 class WebAssemblyTargetCodeGenInfo final : public TargetCodeGenInfo {
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {
+SwiftInfo =
+std::make_unique(CGT, /*SwiftErrorInRegister=*/false);
+  }
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule &CGM) const override {
@@ -1136,7 +1135,7 @@
 };
 

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D130394#3705047 , @inclyc wrote:

> (Not a serious review) It seems like that your patch has already been 
> accepted, so that I can commit this patch for you.

It was very nice of you, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D131225#3706207 , @abidh wrote:

> I think this patch is moving the things in the right direction. But please 
> wait for some other reviewers to chime in.

It seems quite opposite to me.




Comment at: clang/include/clang/Driver/ToolChain.h:384
 
+  /// IsBareMetal - Does this tool chain is a baremetal target.
+  static bool IsBareMetal(const llvm::Triple &);

Is this a correct sentence? (My English is poor.)




Comment at: clang/include/clang/Driver/ToolChain.h:388
+  /// IsRISCVBareMetal - Does this tool chain is a riscv baremetal target.
+  static bool IsRISCVBareMetal(const llvm::Triple &);
+

The ToolChain class is an interface class. It is strange to see such kind of 
methods here. `IsBareMetal` should at least be virtual and overridden in 
concrete implementation of baremetal toolchains. `IsRISCVBareMetal` should not 
be here at all.
What was wrong with the previous implementation that made you move the methods 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

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

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D130689#3706263 , @aaron.ballman 
wrote:

> The failures I am getting are the same as what's shown by the sanitizer bot 
> for Windows: 
> https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
> (I'm using VS 2019 16.11.17 FWIW).

The log says:

> The contents of  are available only with C++17 or later.

if that helps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/include/clang/Driver/ToolChain.h:388
+  /// IsRISCVBareMetal - Does this tool chain is a riscv baremetal target.
+  static bool IsRISCVBareMetal(const llvm::Triple &);
+

manojgupta wrote:
> barannikov88 wrote:
> > The ToolChain class is an interface class. It is strange to see such kind 
> > of methods here. `IsBareMetal` should at least be virtual and overridden in 
> > concrete implementation of baremetal toolchains. `IsRISCVBareMetal` should 
> > not be here at all.
> > What was wrong with the previous implementation that made you move the 
> > methods here?
> There is a need to check for IsBareMetal and variants. They can't be made 
> virtual since the checks need to happen before instantiating ToolChain class. 
> I think moving them to Triple class (Triple.h) is a clearer option.
> I think moving them to Triple class (Triple.h) is a clearer option.
Is the triple is all that is necessary to decide whether the target is bare 
metal or not?
Sounds interesting, but one may argue that Triple should not know about 
toolchains (like it should not know about C data type bit widths, for example).
What if just add a few switch-cases to Driver::getToolChain as for every other 
toolchain? Retaining the former static method 
'BareMetalToolChain::handlesTarget' is still better in my opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/include/clang/Driver/ToolChain.h:388
+  /// IsRISCVBareMetal - Does this tool chain is a riscv baremetal target.
+  static bool IsRISCVBareMetal(const llvm::Triple &);
+

barannikov88 wrote:
> manojgupta wrote:
> > barannikov88 wrote:
> > > The ToolChain class is an interface class. It is strange to see such kind 
> > > of methods here. `IsBareMetal` should at least be virtual and overridden 
> > > in concrete implementation of baremetal toolchains. `IsRISCVBareMetal` 
> > > should not be here at all.
> > > What was wrong with the previous implementation that made you move the 
> > > methods here?
> > There is a need to check for IsBareMetal and variants. They can't be made 
> > virtual since the checks need to happen before instantiating ToolChain 
> > class. I think moving them to Triple class (Triple.h) is a clearer option.
> > I think moving them to Triple class (Triple.h) is a clearer option.
> Is the triple is all that is necessary to decide whether the target is bare 
> metal or not?
> Sounds interesting, but one may argue that Triple should not know about 
> toolchains (like it should not know about C data type bit widths, for 
> example).
> What if just add a few switch-cases to Driver::getToolChain as for every 
> other toolchain? Retaining the former static method 
> 'BareMetalToolChain::handlesTarget' is still better in my opinion.
> They can't be made virtual since the checks need to happen before 
> instantiating ToolChain class. 

This is kind of two different things. It can be made virtual. The name of the 
method `IsBareMetal` of the `ToolChain` class suggests that it is checking 
whether //this concrete instance// of the ToolChain is baremetal. This 
(virtual) method could be used in getCompilerRTPath, for example.
To //instantiate// BareMetalToolchain you need another function (like the 
former BareMetalToolChain::handlesTarget, or the suggested approach with 
Triple). For these instantiations `IsBareMetal` will return true, and false for 
all other instantiations which are not bare metal.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/ADT/Triple.h:916
+  /// Tests if the target is {arm,thumb}-none-none-{eabi,eabihf}.
+  bool isARMBareMetal() const {
+if (getArch() != Triple::arm && getArch() != Triple::thumb)

Any future change to these methods will force recompilation of the whole 
project. It is better to move the definitions into the corresponding cpp file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

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

LGTM with the style issues fixed, but I'd like someone else to take a look.




Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:228-237
+  case ToolChain::RLT_CompilerRT: {
+const std::string fileName = getCompilerRT(Args, "builtins");
+std::string baseName = llvm::sys::path::filename(fileName).str();
+llvm::StringRef baseNameRef(baseName);
+baseNameRef.consume_front("lib");
+baseNameRef.consume_back(".a");
 CmdArgs.push_back(

There are a few style issues.
* `return` is usually within the braces
* Variable names should start with capital letter
* Looks like std::string could be avoided completely in favor of `StringRef` 
and `Twine`

Same applies to ConstructJob below, there are also redundant braces in `for` 
statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/lib/Support/Triple.cpp:1936
+bool Triple::isARMBareMetal() const {
+  if (getArch() != llvm::Triple::arm && getArch() != Triple::thumb)
+return false;

(nit) at least `llvm::` qualification is redundant, `Triple::` might also be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D131225#3707735 , @manojgupta 
wrote:

> Address style nits.

Neat, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131416: [Clang][BinaryOperator] memoize ICEKind for BinaryOperator Exprs

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/include/clang/AST/Expr.h:3832
 
+  llvm::Optional getICEKind() const { return IK; }
+

Does it need to be Optional? It seems that it is never checked that it does not 
hold a value except for the setter method.
An "invalid" enumeration member would serve well IMO.
Not a strong objection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131416

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

[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Still LGTM




Comment at: clang/lib/Driver/ToolChains/BareMetal.h:2
+//===--- BareMetal.h - Bare Metal Tool and ToolChain -*- C++
+//-*-===//
 //

Line overflowed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225

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

[PATCH] D131563: [WIP] [clang] Fix clang multiarch isssue with musl

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

The diff seems to be restricted to few lines. It should include the full 
context.




Comment at: clang/lib/Driver/ToolChains/Linux.cpp:49
 
+  std::string EnvName = TargetTriple.isMusl() ? "musl" : "gnu";
+

* `!isMusl()` does not mean it is "gnu".
* Why don't just use `Triple::getEnvironmentTypeName`?



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

https://reviews.llvm.org/D131563

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

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

@ckissane 
I took fresh sources of LLVM and when I run cmake I get the following warning:

  -- Looking for compress2
  -- Looking for compress2 - found
  CMake Warning at cmake/config-ix.cmake:149 (find_package):
By not providing "Findzstd.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "zstd", but
CMake did not find one.
  
Could not find a package configuration file provided by "zstd" with any of
the following names:
  
  zstdConfig.cmake
  zstd-config.cmake
  
Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
"zstd_DIR" to a directory containing one of the above files.  If "zstd"
provides a separate development package or SDK, be sure it has been
installed.
  Call Stack (most recent call first):
CMakeLists.txt:768 (include)

Ubuntu 22.04 LTS
cmake version 3.22.3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-  CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+  CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+  Packed);

yurai007 wrote:
> nikic wrote:
> > yurai007 wrote:
> > > That's because of "error: conditional expression is ambiguous; 
> > > 'llvm::SmallVector' can be converted to 
> > > 'ArrayRef' and vice versa". Need to check if there is 
> > > easier workaround.
> > Would making the ctor explicit help?
> Nope :( Making constructor explicit disables implicit conversion so we cannot 
> do things like: SmallVector NewMask = Mask; anymore.
And leaving it implicit hides the fact of possible memory allocation, which is 
not cheap. I think absense of such constructor is by-design.

Making it explicit is making it redundant, because there is already a 
constructor which accepts begin / end, and one that accepts range (note that it 
is explicit!). It won't save on typing, either.

I'm not in favor of this patch, but my word does not count much, so I won't 
block it. I'd suggest you, however, to request review of core maintainers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-  CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+  CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+  Packed);

nikic wrote:
> barannikov88 wrote:
> > yurai007 wrote:
> > > nikic wrote:
> > > > yurai007 wrote:
> > > > > That's because of "error: conditional expression is ambiguous; 
> > > > > 'llvm::SmallVector' can be converted to 
> > > > > 'ArrayRef' and vice versa". Need to check if there 
> > > > > is easier workaround.
> > > > Would making the ctor explicit help?
> > > Nope :( Making constructor explicit disables implicit conversion so we 
> > > cannot do things like: SmallVector NewMask = Mask; anymore.
> > And leaving it implicit hides the fact of possible memory allocation, which 
> > is not cheap. I think absense of such constructor is by-design.
> > 
> > Making it explicit is making it redundant, because there is already a 
> > constructor which accepts begin / end, and one that accepts range (note 
> > that it is explicit!). It won't save on typing, either.
> > 
> > I'm not in favor of this patch, but my word does not count much, so I won't 
> > block it. I'd suggest you, however, to request review of core maintainers.
> > Nope :( Making constructor explicit disables implicit conversion so we 
> > cannot do things like: SmallVector NewMask = Mask; anymore.
> 
> I think that's fine. Similar to the existing iterator_range constructor, we 
> would require `SmallVector NewMask(Mask)`, which seems like the 
> idiomatic way to write it anyway?
> 
> > Making it explicit is making it redundant, because there is already a 
> > constructor which accepts begin / end, and one that accepts range (note 
> > that it is explicit!). It won't save on typing, either.
> 
> It is not redundant. It ensures that iterator_range and ArrayRef can be 
> freely substituted. Switching iterator_range to ArrayRef currently requires 
> going through a lot of SmallVector constructors and replacing them with less 
> readable code. The alternative to this change is D129988, which looks like a 
> significant regression in code quality.
Please also consider the fact that this is API breaking change due to this 
"conditional expression is amiguous" error. Many external projects depend on 
LLVM/ADT, and all of them will have to adapt this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130268: [WIP] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-  CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+  CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+  Packed);

barannikov88 wrote:
> nikic wrote:
> > barannikov88 wrote:
> > > yurai007 wrote:
> > > > nikic wrote:
> > > > > yurai007 wrote:
> > > > > > That's because of "error: conditional expression is ambiguous; 
> > > > > > 'llvm::SmallVector' can be converted to 
> > > > > > 'ArrayRef' and vice versa". Need to check if 
> > > > > > there is easier workaround.
> > > > > Would making the ctor explicit help?
> > > > Nope :( Making constructor explicit disables implicit conversion so we 
> > > > cannot do things like: SmallVector NewMask = Mask; anymore.
> > > And leaving it implicit hides the fact of possible memory allocation, 
> > > which is not cheap. I think absense of such constructor is by-design.
> > > 
> > > Making it explicit is making it redundant, because there is already a 
> > > constructor which accepts begin / end, and one that accepts range (note 
> > > that it is explicit!). It won't save on typing, either.
> > > 
> > > I'm not in favor of this patch, but my word does not count much, so I 
> > > won't block it. I'd suggest you, however, to request review of core 
> > > maintainers.
> > > Nope :( Making constructor explicit disables implicit conversion so we 
> > > cannot do things like: SmallVector NewMask = Mask; anymore.
> > 
> > I think that's fine. Similar to the existing iterator_range constructor, we 
> > would require `SmallVector NewMask(Mask)`, which seems like the 
> > idiomatic way to write it anyway?
> > 
> > > Making it explicit is making it redundant, because there is already a 
> > > constructor which accepts begin / end, and one that accepts range (note 
> > > that it is explicit!). It won't save on typing, either.
> > 
> > It is not redundant. It ensures that iterator_range and ArrayRef can be 
> > freely substituted. Switching iterator_range to ArrayRef currently requires 
> > going through a lot of SmallVector constructors and replacing them with 
> > less readable code. The alternative to this change is D129988, which looks 
> > like a significant regression in code quality.
> Please also consider the fact that this is API breaking change due to this 
> "conditional expression is amiguous" error. Many external projects depend on 
> LLVM/ADT, and all of them will have to adapt this change.
> It ensures that iterator_range and ArrayRef can be freely substituted. 
> Switching iterator_range to ArrayRef currently requires going through a lot 
> of SmallVector constructors and replacing them with less readable code. 

Ok, makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130322

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CGObjCRuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -43,10 +43,10 @@
 /// codegeneration issues, like target-specific attributes, builtins and so
 /// on.
 class TargetCodeGenInfo {
-  std::unique_ptr Info = nullptr;
+  std::unique_ptr Info;
 
 public:
-  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
+  TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
 
   /// getABIInfo() - Returns ABI info helper for the target.
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -443,6 +443,9 @@
   return Address(PHI, Addr1.getElementType(), Align);
 }
 
+TargetCodeGenInfo::TargetCodeGenInfo(std::unique_ptr Info)
+: Info(std::move(Info)) {}
+
 TargetCodeGenInfo::~TargetCodeGenInfo() = default;
 
 // If someone can figure out a general rule for this, that would be great.
Index: clang/lib/CodeGen/SwiftCallingConv.cpp
===
--- clang/lib/CodeGen/SwiftCallingConv.cpp
+++ clang/lib/CodeGen/SwiftCallingConv.cpp
@@ -11,9 +11,10 @@
 //===--===//
 
 #include "clang/CodeGen/SwiftCallingConv.h"
-#include "clang/Basic/TargetInfo.h"
+#include "ABIInfo.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
+#include "clang/Basic/TargetInfo.h"
 
 using namespace clang;
 using namespace CodeGen;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "CodeGenModule.h"
+#include "ABIInfo.h"
 #include "CGBlocks.h"
 #include "CGCUDARuntime.h"
 #include "CGCXXABI.h"
@@ -32,7 +33,6 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Mangle.h"
-#include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
Index: clang/lib/CodeGen/CGObjCRuntime.h
===
--- clang/lib/CodeGen/CGObjCRuntime.h
+++ clang/lib/CodeGen/CGObjCRuntime.h
@@ -34,6 +34,7 @@
 
 namespace clang {
 namespace CodeGen {
+  class CGFunctionInfo;
   class CodeGenFunction;
 }
 
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -22,9 +22,6 @@
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 
-// FIXME: Restructure so we don't have to expose so much stuff.
-#include "ABIInfo.h"
-
 namespace llvm {
 class Type;
 class Value;
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "ABIInfo.h"
 #include "CGCUDARuntime.h"
 #include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGCall.h:26
-// FIXME: Restructure so we don't have to expose so much stuff.
-#include "ABIInfo.h"
-

This is the main change. The included file was not used here at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.h:49
 public:
-  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
+  TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();

Had to do this due to "incomplete class" error. Should not change a thing 
because this constructor is ever used in TargetInfo.cpp, where it now resides.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

To reviewers: if you are OK with the patch, could you please merge it? I don't 
have permission yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 446685.
barannikov88 added a comment.

Removed forward declaration of non-existent class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

Files:
  clang/lib/CodeGen/ABIInfo.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CGObjCRuntime.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -43,10 +43,10 @@
 /// codegeneration issues, like target-specific attributes, builtins and so
 /// on.
 class TargetCodeGenInfo {
-  std::unique_ptr Info = nullptr;
+  std::unique_ptr Info;
 
 public:
-  TargetCodeGenInfo(std::unique_ptr Info) : Info(std::move(Info)) {}
+  TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
 
   /// getABIInfo() - Returns ABI info helper for the target.
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -443,6 +443,9 @@
   return Address(PHI, Addr1.getElementType(), Align);
 }
 
+TargetCodeGenInfo::TargetCodeGenInfo(std::unique_ptr Info)
+: Info(std::move(Info)) {}
+
 TargetCodeGenInfo::~TargetCodeGenInfo() = default;
 
 // If someone can figure out a general rule for this, that would be great.
Index: clang/lib/CodeGen/SwiftCallingConv.cpp
===
--- clang/lib/CodeGen/SwiftCallingConv.cpp
+++ clang/lib/CodeGen/SwiftCallingConv.cpp
@@ -11,9 +11,10 @@
 //===--===//
 
 #include "clang/CodeGen/SwiftCallingConv.h"
-#include "clang/Basic/TargetInfo.h"
+#include "ABIInfo.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
+#include "clang/Basic/TargetInfo.h"
 
 using namespace clang;
 using namespace CodeGen;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "CodeGenModule.h"
+#include "ABIInfo.h"
 #include "CGBlocks.h"
 #include "CGCUDARuntime.h"
 #include "CGCXXABI.h"
@@ -32,7 +33,6 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Mangle.h"
-#include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/Builtins.h"
Index: clang/lib/CodeGen/CGObjCRuntime.h
===
--- clang/lib/CodeGen/CGObjCRuntime.h
+++ clang/lib/CodeGen/CGObjCRuntime.h
@@ -34,6 +34,7 @@
 
 namespace clang {
 namespace CodeGen {
+  class CGFunctionInfo;
   class CodeGenFunction;
 }
 
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -22,9 +22,6 @@
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 
-// FIXME: Restructure so we don't have to expose so much stuff.
-#include "ABIInfo.h"
-
 namespace llvm {
 class Type;
 class Value;
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "ABIInfo.h"
 #include "CGCUDARuntime.h"
 #include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
Index: clang/lib/CodeGen/ABIInfo.h
===
--- clang/lib/CodeGen/ABIInfo.h
+++ clang/lib/CodeGen/ABIInfo.h
@@ -35,10 +35,6 @@
   class CodeGenTypes;
   class SwiftABIInfo;
 
-namespace swiftcall {
-  class SwiftAggLowering;
-}
-
   // FIXME: All of this stuff should be part of the target interface
   // somehow. It is currently here because it is not clear how to factor
   // the targets to support this, since the Targets currently live in a
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/ABIInfo.h:39
-namespace swiftcall {
-  class SwiftAggLowering;
-}

Not used in this file. (This class does not seem to be used anywhere.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130268: [NFC] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGExprConstant.cpp:504
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
-  CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);
+  CGM.getLLVMContext(), Packed ? PackedElems : to_vector(UnpackedElems),
+  Packed);

yurai007 wrote:
> yurai007 wrote:
> > barannikov88 wrote:
> > > barannikov88 wrote:
> > > > nikic wrote:
> > > > > barannikov88 wrote:
> > > > > > yurai007 wrote:
> > > > > > > nikic wrote:
> > > > > > > > yurai007 wrote:
> > > > > > > > > That's because of "error: conditional expression is 
> > > > > > > > > ambiguous; 'llvm::SmallVector' can be 
> > > > > > > > > converted to 'ArrayRef' and vice versa". 
> > > > > > > > > Need to check if there is easier workaround.
> > > > > > > > Would making the ctor explicit help?
> > > > > > > Nope :( Making constructor explicit disables implicit conversion 
> > > > > > > so we cannot do things like: SmallVector NewMask = Mask; 
> > > > > > > anymore.
> > > > > > And leaving it implicit hides the fact of possible memory 
> > > > > > allocation, which is not cheap. I think absense of such constructor 
> > > > > > is by-design.
> > > > > > 
> > > > > > Making it explicit is making it redundant, because there is already 
> > > > > > a constructor which accepts begin / end, and one that accepts range 
> > > > > > (note that it is explicit!). It won't save on typing, either.
> > > > > > 
> > > > > > I'm not in favor of this patch, but my word does not count much, so 
> > > > > > I won't block it. I'd suggest you, however, to request review of 
> > > > > > core maintainers.
> > > > > > Nope :( Making constructor explicit disables implicit conversion so 
> > > > > > we cannot do things like: SmallVector NewMask = Mask; 
> > > > > > anymore.
> > > > > 
> > > > > I think that's fine. Similar to the existing iterator_range 
> > > > > constructor, we would require `SmallVector NewMask(Mask)`, 
> > > > > which seems like the idiomatic way to write it anyway?
> > > > > 
> > > > > > Making it explicit is making it redundant, because there is already 
> > > > > > a constructor which accepts begin / end, and one that accepts range 
> > > > > > (note that it is explicit!). It won't save on typing, either.
> > > > > 
> > > > > It is not redundant. It ensures that iterator_range and ArrayRef can 
> > > > > be freely substituted. Switching iterator_range to ArrayRef currently 
> > > > > requires going through a lot of SmallVector constructors and 
> > > > > replacing them with less readable code. The alternative to this 
> > > > > change is D129988, which looks like a significant regression in code 
> > > > > quality.
> > > > Please also consider the fact that this is API breaking change due to 
> > > > this "conditional expression is amiguous" error. Many external projects 
> > > > depend on LLVM/ADT, and all of them will have to adapt this change.
> > > > It ensures that iterator_range and ArrayRef can be freely substituted. 
> > > > Switching iterator_range to ArrayRef currently requires going through a 
> > > > lot of SmallVector constructors and replacing them with less readable 
> > > > code. 
> > > 
> > > Ok, makes sense.
> > > Nope :( Making constructor explicit disables implicit conversion so 
> > > we cannot do things like: SmallVector NewMask = Mask; anymore.
> > > 
> > > I think that's fine. Similar to the existing iterator_range constructor, 
> > > we would require SmallVector NewMask(Mask), which seems like the 
> > > idiomatic way to write it anyway?
> > 
> > You are right, explicit works when we use brackets instead assignment. I'm 
> > gonna add it to constructor and adjust rest of patch.
> @barannikov88: after addressing @nikic comment there is no API breaking 
> change anymore. As far I can tell SmallVector API is only extended and still 
> backward-compatible. Hope it looks better now.
Looks good, thank you. Still, I would suggest requesting a review from more 
experienced developers than me.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130268: [NFC] Add SmallVector constructor to allow creation of SmallVector from ArrayRef of items convertible to type T

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/ADT/SmallVector.h:35
 
+template  class ArrayRef;
+

Should be `typename T`. `class T` is archaic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130268

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

@MaskRay 
Would you please merge it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130322: [clang][CodeGen] Only include ABIInfo.h where required (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D130322#3672362 , @MaskRay wrote:

> Testing and will push in one minute

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130322

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

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
Herald added a subscriber: dschuff.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added a project: clang.

Swift calling conventions stands out in the way that they are lowered in
mostly target-independent manner, with very few customization points.
As such, swift-related methods of ABIInfo do not reference the rest of
ABIInfo and vice versa.
This change follows interface segregation principle; it factors out
swift-related stuff into separate class -- SwiftABIInfo -- which
targets must implement if they support Swift calling conventions.

Almost all targets implemented `shouldPassIndirectly` the same way. This
de-facto default implementation has been moved into the base class.

`isSwiftErrorInRegister` used to be virtual, now it is not. It didn't
accept any arguments which could have an effect on the returned value.
This is now a static property of the target ABI.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130394

Files:
  clang/lib/CodeGen/ABIInfo.h
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -38,6 +38,7 @@
 class CallArgList;
 class CodeGenFunction;
 class CGBlockInfo;
+class SwiftABIInfo;
 
 /// TargetCodeGenInfo - This class organizes various target-specific
 /// codegeneration issues, like target-specific attributes, builtins and so
@@ -45,6 +46,12 @@
 class TargetCodeGenInfo {
   std::unique_ptr Info;
 
+protected:
+  // Target hooks supporting Swift calling conventions. The target must
+  // initialize this field if it claims to support these calling conventions
+  // by returning true from TargetInfo::checkCallingConvention for them.
+  std::unique_ptr SwiftInfo;
+
 public:
   TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
@@ -52,6 +59,9 @@
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo &getABIInfo() const { return *Info; }
 
+  /// Returns Swift ABI info helper for the target.
+  const SwiftABIInfo &getSwiftABIInfo() const { return *SwiftInfo; }
+
   /// setTargetAttributes - Provides a convenient hook to handle extra
   /// target-specific attributes for the given global.
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -23,7 +23,6 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
-#include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -117,7 +116,9 @@
   return false;
 }
 
-ABIInfo::~ABIInfo() {}
+ABIInfo::~ABIInfo() = default;
+
+SwiftABIInfo::~SwiftABIInfo() = default;
 
 /// Does the given lowering require more than the given number of
 /// registers when expanded?
@@ -151,12 +152,16 @@
   return (intCount + fpCount > maxAllRegisters);
 }
 
-bool SwiftABIInfo::isLegalVectorTypeForSwift(CharUnits vectorSize,
- llvm::Type *eltTy,
- unsigned numElts) const {
+bool SwiftABIInfo::shouldPassIndirectly(ArrayRef ComponentTys,
+bool AsReturnValue) const {
+  return occupiesMoreThan(CGT, ComponentTys, /*total=*/4);
+}
+
+bool SwiftABIInfo::isLegalVectorType(CharUnits VectorSize, llvm::Type *EltTy,
+ unsigned NumElts) const {
   // The default implementation of this assumes that the target guarantees
   // 128-bit SIMD support but nothing more.
-  return (vectorSize.getQuantity() > 8 && vectorSize.getQuantity() <= 16);
+  return (VectorSize.getQuantity() > 8 && VectorSize.getQuantity() <= 16);
 }
 
 static CGCXXABI::RecordArgABI getRecordArgABI(const RecordType *RT,
@@ -814,7 +819,7 @@
 // This is a very simple ABI that relies a lot on DefaultABIInfo.
 //===--===//
 
-class WebAssemblyABIInfo final : public SwiftABIInfo {
+class WebAssemblyABIInfo final : public ABIInfo {
 public:
   enum ABIKind {
 MVP = 0,
@@ -827,7 +832,7 @@
 
 public:
   explicit WebAssemblyABIInfo(CodeGen::CodeGenTypes &CGT, ABIKind Kind)
-  : SwiftABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
+  : ABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
 
 private:
   ABIArgInfo classifyReturnType(QualType RetTy) const;
@@ -845,22 +850,16 @@
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType Ty) const override;
-
-  bool shouldPass

[PATCH] D130394: [clang][CodeGen] Factor out Swift ABI hooks

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 446953.
barannikov88 added a comment.

Reword commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

Files:
  clang/lib/CodeGen/ABIInfo.h
  clang/lib/CodeGen/SwiftCallingConv.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h

Index: clang/lib/CodeGen/TargetInfo.h
===
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -38,6 +38,7 @@
 class CallArgList;
 class CodeGenFunction;
 class CGBlockInfo;
+class SwiftABIInfo;
 
 /// TargetCodeGenInfo - This class organizes various target-specific
 /// codegeneration issues, like target-specific attributes, builtins and so
@@ -45,6 +46,12 @@
 class TargetCodeGenInfo {
   std::unique_ptr Info;
 
+protected:
+  // Target hooks supporting Swift calling conventions. The target must
+  // initialize this field if it claims to support these calling conventions
+  // by returning true from TargetInfo::checkCallingConvention for them.
+  std::unique_ptr SwiftInfo;
+
 public:
   TargetCodeGenInfo(std::unique_ptr Info);
   virtual ~TargetCodeGenInfo();
@@ -52,6 +59,9 @@
   /// getABIInfo() - Returns ABI info helper for the target.
   const ABIInfo &getABIInfo() const { return *Info; }
 
+  /// Returns Swift ABI info helper for the target.
+  const SwiftABIInfo &getSwiftABIInfo() const { return *SwiftInfo; }
+
   /// setTargetAttributes - Provides a convenient hook to handle extra
   /// target-specific attributes for the given global.
   virtual void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -23,7 +23,6 @@
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
-#include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -117,7 +116,9 @@
   return false;
 }
 
-ABIInfo::~ABIInfo() {}
+ABIInfo::~ABIInfo() = default;
+
+SwiftABIInfo::~SwiftABIInfo() = default;
 
 /// Does the given lowering require more than the given number of
 /// registers when expanded?
@@ -151,12 +152,16 @@
   return (intCount + fpCount > maxAllRegisters);
 }
 
-bool SwiftABIInfo::isLegalVectorTypeForSwift(CharUnits vectorSize,
- llvm::Type *eltTy,
- unsigned numElts) const {
+bool SwiftABIInfo::shouldPassIndirectly(ArrayRef ComponentTys,
+bool AsReturnValue) const {
+  return occupiesMoreThan(CGT, ComponentTys, /*total=*/4);
+}
+
+bool SwiftABIInfo::isLegalVectorType(CharUnits VectorSize, llvm::Type *EltTy,
+ unsigned NumElts) const {
   // The default implementation of this assumes that the target guarantees
   // 128-bit SIMD support but nothing more.
-  return (vectorSize.getQuantity() > 8 && vectorSize.getQuantity() <= 16);
+  return (VectorSize.getQuantity() > 8 && VectorSize.getQuantity() <= 16);
 }
 
 static CGCXXABI::RecordArgABI getRecordArgABI(const RecordType *RT,
@@ -814,7 +819,7 @@
 // This is a very simple ABI that relies a lot on DefaultABIInfo.
 //===--===//
 
-class WebAssemblyABIInfo final : public SwiftABIInfo {
+class WebAssemblyABIInfo final : public ABIInfo {
 public:
   enum ABIKind {
 MVP = 0,
@@ -827,7 +832,7 @@
 
 public:
   explicit WebAssemblyABIInfo(CodeGen::CodeGenTypes &CGT, ABIKind Kind)
-  : SwiftABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
+  : ABIInfo(CGT), defaultInfo(CGT), Kind(Kind) {}
 
 private:
   ABIArgInfo classifyReturnType(QualType RetTy) const;
@@ -845,22 +850,16 @@
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
 QualType Ty) const override;
-
-  bool shouldPassIndirectlyForSwift(ArrayRef scalars,
-bool asReturnValue) const override {
-return occupiesMoreThan(CGT, scalars, /*total*/ 4);
-  }
-
-  bool isSwiftErrorInRegister() const override {
-return false;
-  }
 };
 
 class WebAssemblyTargetCodeGenInfo final : public TargetCodeGenInfo {
 public:
   explicit WebAssemblyTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT,
 WebAssemblyABIInfo::ABIKind K)
-  : TargetCodeGenInfo(std::make_unique(CGT, K)) {}
+  : TargetCodeGenInfo(std::make_unique(CGT, K)) {
+SwiftInfo =
+std::make_unique(CGT, /*SwiftErrorInRegister=*/false);
+  }
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule &CGM) const override {
@@ -1136,7 +

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Just thoughts.

llvm::any_isa is usually paired with llvm::any_cast; replacing them with 
llvm::any_cast and nullptr check seems fine.
However,

- std::any uses RTTI, so it is unlikely to ever replace llvm::Any.
- llvm::any_isa<> is kind of convenient and is aligned with llvm::isa<>. Same 
for llvm::any_cast.
- With that in mind, introducing new llvm::any_cast_or_null to follow 
llvm::cast_or_null instead of changing the semantics of llvm::any_cast might be 
a better idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: lldb/include/lldb/Core/RichManglingContext.h:90-91
 assert(parser.has_value());
-assert(llvm::any_isa(parser));
+assert(llvm::any_cast(&parser));
 return llvm::any_cast(parser);
   }

This is not intuitively clear. In assert you pass an address, but in 'return' 
below the object is passed by reference.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D139973#4004408 , @jsilvanus wrote:

> I just checked on Godbolt that gcc 12.2 with libstdc++, clang 15.0 with both 
> libstd++ and libc++ and MSVC 19.33 support `std::any` without RTTI. 
> But gcc and clang do not provide `std::any::type` without RTTI though.
>
> Are you aware of any documentation on what can be relied on without RTTI?

It is surprising to me that std::any can work without RTTI. Never thought it 
could be implemented.
This "compare function pointer" trick is awesome, but it might not always work 
as explained in this 

 commit.
This question 

 however, suggest that any_cast doesn't always work with RTTI either, which is 
weird.
I don't know of any other potential issues. std::visitor can't be used with 
std::any in absense of std::any::type, but that's minor, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:193
 
-  if (any_isa(IR)) {
-const Function *F = any_cast(IR);
-return F->getName().str();
+  if (const auto **F = any_cast(&IR)) {
+return (*F)->getName().str();

Redundant braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Related: https://reviews.llvm.org/D139974


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 accepted this revision.
barannikov88 added a comment.

In D139973#4005120 , @sebastian-ne 
wrote:

> the question gets wether we want to keep llvm::Any around as a wrapper of 
> std::any once we can use it (in this case this patch would be obsolete)
> or if we we want to remove llvm::Any and use std::any only.

Since there are not many uses of Any, it is hard to say which option would be 
preferable in the long term.
Perhaps, it would be possible to remove llvm::Any and use std::any with 
llvm::any_isa as an extension.
Anyway, this patch makes it possible to avoid double casting, which is an 
improvement by itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/Basic/TargetInfo.cpp:513
   if (Opts.MaxBitIntWidth)
-MaxBitIntWidth = Opts.MaxBitIntWidth;
+MaxBitIntWidth = (unsigned)Opts.MaxBitIntWidth;
 

Nit: C-style casts are generally discouraged (there are several occurrences in 
this patch).



Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:83
   public:
-ExtraRegInfo() = default;
+ExtraRegInfo() {}
 ExtraRegInfo(const ExtraRegInfo &) = delete;

Is it somehow different than ' = default'?



Comment at: mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp:182
 
-  return ret;
+  return std::move(ret);
 }

clang-tidy would usually complain on this. Does it solve some gcc issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 accepted this revision.
barannikov88 added inline comments.



Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:83
   public:
-ExtraRegInfo() = default;
+ExtraRegInfo() {}
 ExtraRegInfo(const ExtraRegInfo &) = delete;

bkramer wrote:
> barannikov88 wrote:
> > Is it somehow different than ' = default'?
> It makes the class non-trivial, std::optional::emplace has issues with 
> trivial default constructors :(
Ah, right, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[PATCH] D137838: [Support] Move TargetParsers to new component

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/ADT/Triple.h:11
+/// This header is deprecated in favour of
+/// `llvm/TargetParser/AArch64TargetParser.h`.
 ///

Invalid comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in:17
 
-#define LLVM_LIBDIR_SUFFIX "@LLVM_LIBDIR_SUFFIX@"
+#define CMAKE_INSTALL_LIBDIR "@CMAKE_INSTALL_LIBDIR@"

The prefix must remain LLVM_*
In other components accordingly


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

https://reviews.llvm.org/D137337

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

[PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in:17
 
-#define LLVM_LIBDIR_SUFFIX "@LLVM_LIBDIR_SUFFIX@"
+#define CMAKE_INSTALL_LIBDIR "@CMAKE_INSTALL_LIBDIR@"

barannikov88 wrote:
> The prefix must remain LLVM_*
> In other components accordingly
> The prefix must remain LLVM_*
> In other components accordingly

To clarify, I mean the C++ macro name and not CMake variable name


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

https://reviews.llvm.org/D137337

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

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14742
 Builder.getFastMathFlags().setAllowReassoc();
-return Builder.CreateCall(F, {Ops[0], Ops[1]});
+Value *FAdd = Builder.CreateCall(F, {Ops[0], Ops[1]});
+Builder.getFastMathFlags() &= (FMF);

Could be just:
```
return Builder.CreateCall(F, {Ops[0], Ops[1]})
->getFastMathFlags()
.setAllowReassoc();
```
so that you don't have to save and restore Builder's flags.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140467

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

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14742
 Builder.getFastMathFlags().setAllowReassoc();
-return Builder.CreateCall(F, {Ops[0], Ops[1]});
+Value *FAdd = Builder.CreateCall(F, {Ops[0], Ops[1]});
+Builder.getFastMathFlags() &= (FMF);

barannikov88 wrote:
> Could be just:
> ```
> return Builder.CreateCall(F, {Ops[0], Ops[1]})
> ->getFastMathFlags()
> .setAllowReassoc();
> ```
> so that you don't have to save and restore Builder's flags.
> 
No, it couldn't be, just tried :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140467

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/lib/TargetParser/CMakeLists.txt:29
+# LLVMTargetParser. See https://stackoverflow.com/a/25681179
+target_include_directories(LLVMTargetParser PUBLIC 
$)

Will it work if RISC-V target is not compiled-in?
This does not strictly add a cyclic dependency, but it would still be nice to 
avoid dependency on higher-level components.
Is it possible / reasonable to extract the part of the RISCV.td that relates to 
this component and put it separate td file in this directory? Or is it tightly 
coupled with the rest of the target description?




Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:15
+#include "llvm/TableGen/Record.h"
+#include 
+namespace llvm {

 [[ 
https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden | is 
forbidden ]] by the coding standards.




Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:16
+#include 
+namespace llvm {
+

This [[ 
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
 | should be ]] `using namespace llvm;`



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:18
+
+std::string getEnumFeatures(Record &Rec) {
+  std::vector Features = Rec.getValueAsListOfDefs("Features");

This should be `static`.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:20
+  std::vector Features = Rec.getValueAsListOfDefs("Features");
+  if (std::find_if(Features.begin(), Features.end(), [](Record *R) {
+return R->getName() == "Feature64Bit";

(suggestion) LLVM's version of find_if accepts ranges, which is a bit shorter.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:28
+
+void EmitRISCVTargetDef(RecordKeeper &RK, raw_ostream &OS) {
+  const auto &Map = RK.getDefs();

This function does not seem to mutate RecordKeeper, so it should be `const`.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:37
+  // Iterate on all definition records.
+  for (auto &Def : Map) {
+const auto &Record = Def.second;

Should also `const`, same for the loop below.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:52
+  for (auto &Def : Map) {
+const auto &Record = Def.second;
+if (Record->isSubClassOf("RISCVProcessorModelTUNE_PROC"))

Same for the loop above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:16
+#include 
+namespace llvm {
+

fpetrogalli wrote:
> barannikov88 wrote:
> > This [[ 
> > https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
> >  | should be ]] `using namespace llvm;`
> Hum, if I do this, I get:
> 
> ```
> Undefined symbols for architecture arm64:
>   "llvm::EmitRISCVTargetDef(llvm::RecordKeeper&, llvm::raw_ostream&)", 
> referenced from:
>   (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, 
> llvm::RecordKeeper&) in TableGen.cpp.o
> ld: symbol(s) not found for architecture arm64
> ```
> 
> It is a bit surprising because the linking command has 
> `utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVTargetDefEmitter.cpp.o` into 
> it... Some of the files in this folder do not use the convention you pointed 
> at, it it OK if I live it as it is?
Right, after `using namespace llvm` you have to write
`llvm::EmitRISCVTargetDef` with explicit `llvm::` qualification. This is the 
whole point of this guideline :)
Please see the [[ 
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
 | link ]].



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:52
+  for (auto &Def : Map) {
+const auto &Record = Def.second;
+if (Record->isSubClassOf("RISCVProcessorModelTUNE_PROC"))

fpetrogalli wrote:
> barannikov88 wrote:
> > Same for the loop above.
> ```
> /Users/fpetrogalli/projects/cpu-defs/upstream/llvm-project/llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:38:17:
>  error: variable 'Record' with type 'const auto *' has incompatible 
> initializer of type 'const std::unique_ptr'
> const auto *Record = Def.second;
> ```
Whoops, my bad, sorry.
Never liked these 'auto's... Never know what is behind them. Looks like a raw 
pointer, but it is not.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D142459: [clang] Deprecate uses of GlobalObject::getAlignment

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:491
   new llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,
- llvm::Align(Var->getAlignment()), I);
+ Var->getAlign().valueOrOne(), I);
   WorkItem.pop_back();

tra wrote:
> gchatelet wrote:
> > tra wrote:
> > > This appears to be a change in behavior. AFAICT, previously used 
> > > Var->getAlignment() could return alignment value or 0. Now it's value or 
> > > 1.
> > > 
> > > Is it intentional?
> > The previous statement was constructing an `llvm::Align` from a value, and 
> > `llvm::Align` [asserts when the provided value is 
> > 0](https://github.com/llvm/llvm-project/blob/4ab2246d486ba30c4b2d654323a0c0b97565c0f1/llvm/include/llvm/Support/Alignment.h#L76-L81).
> >  This means that it is undefined to pass the value `0`.
> > 
> > As far as `LoadInst` is concerned it can only accept valid alignments 
> > (i.e., powers of two => non zero).
> > 
> > So you're right that it is not strictly NFC and that 
> > `*Var->getAlign()`would be a more rigorous transformation but I //think// 
> > that converting the `0` value to `1` moves us from UB to semantically valid 
> > code.
> > 
> > I don't feel strongly about it though and I'm fine changing this to 
> > `*Var->getAlign()` to keep this patch NFC. WDYT?
> Enforcing alignment of 1 would potentially force us to generate overly 
> conservative one byte at a time loads/stores.
> I agree that passing 0 is a wrong choice here, but 1 does not seem to be 
> correct, either.
> Unfortunately LoadInst does not have overloads accepting MaybeAlign so we 
> need to use different `LoadInst` overload depending on whether alignment is 
> specified.
> 
> ```
> NewV =  Var->getAlign().isAligned() 
>   ? llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  
> Var->getAlign().value(), I)
>   : llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  I);
> ```
> 
> @yaxunl -- Sam, does it make sense? This seems to be largely HIP-specific.
I think it should be just `Var->getAlign()` to allow propagating absent 
alignment.
Curiously, the old code didn't assert because `GlobalVariable` seem to 
always(?) have non-empty alignment if the global had an associated `VarDecl` 
(set [[ 
https://github.com/llvm/llvm-project/blob/6ad0788c332bb2043142954d300c49ac3e537f34/clang/lib/CodeGen/CodeGenModule.cpp#L4442
 | here ]], changed(?) by [[ 
https://github.com/llvm/llvm-project/commit/c79099e0f44d0f85515fd30c83923d9d9dc1679b
 | this patch ]]).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142459

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

[PATCH] D142459: [clang] Deprecate uses of GlobalObject::getAlignment

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGCUDANV.cpp:491
   new llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,
- llvm::Align(Var->getAlignment()), I);
+ Var->getAlign().valueOrOne(), I);
   WorkItem.pop_back();

barannikov88 wrote:
> tra wrote:
> > gchatelet wrote:
> > > tra wrote:
> > > > This appears to be a change in behavior. AFAICT, previously used 
> > > > Var->getAlignment() could return alignment value or 0. Now it's value 
> > > > or 1.
> > > > 
> > > > Is it intentional?
> > > The previous statement was constructing an `llvm::Align` from a value, 
> > > and `llvm::Align` [asserts when the provided value is 
> > > 0](https://github.com/llvm/llvm-project/blob/4ab2246d486ba30c4b2d654323a0c0b97565c0f1/llvm/include/llvm/Support/Alignment.h#L76-L81).
> > >  This means that it is undefined to pass the value `0`.
> > > 
> > > As far as `LoadInst` is concerned it can only accept valid alignments 
> > > (i.e., powers of two => non zero).
> > > 
> > > So you're right that it is not strictly NFC and that 
> > > `*Var->getAlign()`would be a more rigorous transformation but I //think// 
> > > that converting the `0` value to `1` moves us from UB to semantically 
> > > valid code.
> > > 
> > > I don't feel strongly about it though and I'm fine changing this to 
> > > `*Var->getAlign()` to keep this patch NFC. WDYT?
> > Enforcing alignment of 1 would potentially force us to generate overly 
> > conservative one byte at a time loads/stores.
> > I agree that passing 0 is a wrong choice here, but 1 does not seem to be 
> > correct, either.
> > Unfortunately LoadInst does not have overloads accepting MaybeAlign so we 
> > need to use different `LoadInst` overload depending on whether alignment is 
> > specified.
> > 
> > ```
> > NewV =  Var->getAlign().isAligned() 
> >   ? llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  
> > Var->getAlign().value(), I)
> >   : llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,  I);
> > ```
> > 
> > @yaxunl -- Sam, does it make sense? This seems to be largely HIP-specific.
> I think it should be just `Var->getAlign()` to allow propagating absent 
> alignment.
> Curiously, the old code didn't assert because `GlobalVariable` seem to 
> always(?) have non-empty alignment if the global had an associated `VarDecl` 
> (set [[ 
> https://github.com/llvm/llvm-project/blob/6ad0788c332bb2043142954d300c49ac3e537f34/clang/lib/CodeGen/CodeGenModule.cpp#L4442
>  | here ]], changed(?) by [[ 
> https://github.com/llvm/llvm-project/commit/c79099e0f44d0f85515fd30c83923d9d9dc1679b
>  | this patch ]]).
> 
> Unfortunately LoadInst does not have overloads accepting MaybeAlign so we 
> need to use different `LoadInst` overload depending on whether alignment is 
> specified.

That's interesting, because `SelectionDAG::getLoad` has many variants with 
`MaybeAlign`, but only one with `Align`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142459

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

[PATCH] D142968: [NFC] Extract `CodeGenInstAlias` into its own *.h/*.cpp

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142968

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/TableGen/AsmMatcherEmitter.cpp
  llvm/utils/TableGen/AsmWriterEmitter.cpp
  llvm/utils/TableGen/CMakeLists.txt
  llvm/utils/TableGen/CodeGenInstAlias.cpp
  llvm/utils/TableGen/CodeGenInstAlias.h
  llvm/utils/TableGen/CodeGenInstruction.cpp
  llvm/utils/TableGen/CodeGenInstruction.h
  llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
@@ -15,6 +15,7 @@
 "CodeEmitterGen.cpp",
 "CodeGenDAGPatterns.cpp",
 "CodeGenHwModes.cpp",
+"CodeGenInstAlias.cpp",
 "CodeGenInstruction.cpp",
 "CodeGenMapTable.cpp",
 "CodeGenRegisters.cpp",
Index: llvm/utils/TableGen/CodeGenInstruction.h
===
--- llvm/utils/TableGen/CodeGenInstruction.h
+++ llvm/utils/TableGen/CodeGenInstruction.h
@@ -23,8 +23,6 @@
 #include 
 
 namespace llvm {
-class SMLoc;
-template  class ArrayRef;
   class Record;
   class DagInit;
   class CodeGenTarget;
@@ -340,71 +338,6 @@
 bool isOperandImpl(StringRef OpListName, unsigned i,
StringRef PropertyName) const;
   };
-
-
-  /// CodeGenInstAlias - This represents an InstAlias definition.
-  class CodeGenInstAlias {
-  public:
-Record *TheDef;// The actual record defining this InstAlias.
-
-/// AsmString - The format string used to emit a .s file for the
-/// instruction.
-std::string AsmString;
-
-/// Result - The result instruction.
-DagInit *Result;
-
-/// ResultInst - The instruction generated by the alias (decoded from
-/// Result).
-CodeGenInstruction *ResultInst;
-
-
-struct ResultOperand {
-private:
-  std::string Name;
-  Record *R = nullptr;
-  int64_t Imm = 0;
-
-public:
-  enum {
-K_Record,
-K_Imm,
-K_Reg
-  } Kind;
-
-  ResultOperand(std::string N, Record *r)
-  : Name(std::move(N)), R(r), Kind(K_Record) {}
-  ResultOperand(int64_t I) : Imm(I), Kind(K_Imm) {}
-  ResultOperand(Record *r) : R(r), Kind(K_Reg) {}
-
-  bool isRecord() const { return Kind == K_Record; }
-  bool isImm() const { return Kind == K_Imm; }
-  bool isReg() const { return Kind == K_Reg; }
-
-  StringRef getName() const { assert(isRecord()); return Name; }
-  Record *getRecord() const { assert(isRecord()); return R; }
-  int64_t getImm() const { assert(isImm()); return Imm; }
-  Record *getRegister() const { assert(isReg()); return R; }
-
-  unsigned getMINumOperands() const;
-};
-
-/// ResultOperands - The decoded operands for the result instruction.
-std::vector ResultOperands;
-
-/// ResultInstOperandIndex - For each operand, this vector holds a pair of
-/// indices to identify the corresponding operand in the result
-/// instruction.  The first index specifies the operand and the second
-/// index specifies the suboperand.  If there are no suboperands or if all
-/// of them are matched by the operand, the second value should be -1.
-std::vector > ResultInstOperandIndex;
-
-CodeGenInstAlias(Record *R, CodeGenTarget &T);
-
-bool tryAliasOpMatch(DagInit *Result, unsigned AliasOpNo,
- Record *InstOpRec, bool hasSubOps, ArrayRef Loc,
- CodeGenTarget &T, ResultOperand &ResOp);
-  };
-}
+} // namespace llvm
 
 #endif
Index: llvm/utils/TableGen/CodeGenInstruction.cpp
===
--- llvm/utils/TableGen/CodeGenInstruction.cpp
+++ llvm/utils/TableGen/CodeGenInstruction.cpp
@@ -13,7 +13,6 @@
 #include "CodeGenInstruction.h"
 #include "CodeGenTarget.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/ADT/StringMap.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include 
@@ -592,266 +591,3 @@
   return Constraint->getDef()->isSubClassOf("TypedOperand") &&
  Constraint->getDef()->getValueAsBit(PropertyName);
 }
-
-//===--===//
-/// CodeGenInstAlias Implementation
-//===--===//
-
-/// tryAliasOpMatch - This is a helper function for the CodeGenInstAlias
-/// constructor.  It checks if an argument in an InstAlias pattern matches
-/// the corresponding operand of the instruction.  It returns true on a
-/// successful match, with ResOp 

[PATCH] D142968: [NFC] Extract `CodeGenInstAlias` into its own *.h/*.cpp

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

I intent to make make InstAlias with "complex" operands (non-empty 
MIOperandInfo) more robust (stricter syntax).
The change will clobber most of the code here, and I thought it is a good time 
to move the class to a dedicated file.
Another weak point is that this class is only used in AsmMatcherEmitter and 
AsmWriterEmitter, while CodeGenInstruction.h is included everywhere.
The class does not have long history; there were only a few functional changes 
since initial commit.

That is, no strong motivation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142968

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

[PATCH] D142968: [NFC] Extract `CodeGenInstAlias` into its own *.h/*.cpp

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7fbca40805c: [NFC] Extract `CodeGenInstAlias` into its own 
*.h/*.cpp (authored by barannikov88).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142968

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/TableGen/AsmMatcherEmitter.cpp
  llvm/utils/TableGen/AsmWriterEmitter.cpp
  llvm/utils/TableGen/CMakeLists.txt
  llvm/utils/TableGen/CodeGenInstAlias.cpp
  llvm/utils/TableGen/CodeGenInstAlias.h
  llvm/utils/TableGen/CodeGenInstruction.cpp
  llvm/utils/TableGen/CodeGenInstruction.h
  llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
@@ -15,6 +15,7 @@
 "CodeEmitterGen.cpp",
 "CodeGenDAGPatterns.cpp",
 "CodeGenHwModes.cpp",
+"CodeGenInstAlias.cpp",
 "CodeGenInstruction.cpp",
 "CodeGenMapTable.cpp",
 "CodeGenRegisters.cpp",
Index: llvm/utils/TableGen/CodeGenInstruction.h
===
--- llvm/utils/TableGen/CodeGenInstruction.h
+++ llvm/utils/TableGen/CodeGenInstruction.h
@@ -23,8 +23,6 @@
 #include 
 
 namespace llvm {
-class SMLoc;
-template  class ArrayRef;
   class Record;
   class DagInit;
   class CodeGenTarget;
@@ -340,71 +338,6 @@
 bool isOperandImpl(StringRef OpListName, unsigned i,
StringRef PropertyName) const;
   };
-
-
-  /// CodeGenInstAlias - This represents an InstAlias definition.
-  class CodeGenInstAlias {
-  public:
-Record *TheDef;// The actual record defining this InstAlias.
-
-/// AsmString - The format string used to emit a .s file for the
-/// instruction.
-std::string AsmString;
-
-/// Result - The result instruction.
-DagInit *Result;
-
-/// ResultInst - The instruction generated by the alias (decoded from
-/// Result).
-CodeGenInstruction *ResultInst;
-
-
-struct ResultOperand {
-private:
-  std::string Name;
-  Record *R = nullptr;
-  int64_t Imm = 0;
-
-public:
-  enum {
-K_Record,
-K_Imm,
-K_Reg
-  } Kind;
-
-  ResultOperand(std::string N, Record *r)
-  : Name(std::move(N)), R(r), Kind(K_Record) {}
-  ResultOperand(int64_t I) : Imm(I), Kind(K_Imm) {}
-  ResultOperand(Record *r) : R(r), Kind(K_Reg) {}
-
-  bool isRecord() const { return Kind == K_Record; }
-  bool isImm() const { return Kind == K_Imm; }
-  bool isReg() const { return Kind == K_Reg; }
-
-  StringRef getName() const { assert(isRecord()); return Name; }
-  Record *getRecord() const { assert(isRecord()); return R; }
-  int64_t getImm() const { assert(isImm()); return Imm; }
-  Record *getRegister() const { assert(isReg()); return R; }
-
-  unsigned getMINumOperands() const;
-};
-
-/// ResultOperands - The decoded operands for the result instruction.
-std::vector ResultOperands;
-
-/// ResultInstOperandIndex - For each operand, this vector holds a pair of
-/// indices to identify the corresponding operand in the result
-/// instruction.  The first index specifies the operand and the second
-/// index specifies the suboperand.  If there are no suboperands or if all
-/// of them are matched by the operand, the second value should be -1.
-std::vector > ResultInstOperandIndex;
-
-CodeGenInstAlias(Record *R, CodeGenTarget &T);
-
-bool tryAliasOpMatch(DagInit *Result, unsigned AliasOpNo,
- Record *InstOpRec, bool hasSubOps, ArrayRef Loc,
- CodeGenTarget &T, ResultOperand &ResOp);
-  };
-}
+} // namespace llvm
 
 #endif
Index: llvm/utils/TableGen/CodeGenInstruction.cpp
===
--- llvm/utils/TableGen/CodeGenInstruction.cpp
+++ llvm/utils/TableGen/CodeGenInstruction.cpp
@@ -13,7 +13,6 @@
 #include "CodeGenInstruction.h"
 #include "CodeGenTarget.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/ADT/StringMap.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include 
@@ -592,266 +591,3 @@
   return Constraint->getDef()->isSubClassOf("TypedOperand") &&
  Constraint->getDef()->getValueAsBit(PropertyName);
 }
-
-//===--===//
-/// CodeGenInstAlias Implementation
-//===--===//
-
-/// tryAliasOpMatch - This is a helper function for the CodeGenInstAlias
-/// constructor.  It checks if an argument in an InstAlias pattern matches
-/// the corresponding operand of the in

[PATCH] D141788: [NFC] Use `llvm::enumerate` in llvm/unittests/Object

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
Herald added subscribers: s.egerton, simoncook, fedor.sergeev.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, pcwang-thead.
Herald added projects: clang, LLVM.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141788

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/unittests/Object/ELFObjectFileTest.cpp

Index: llvm/unittests/Object/ELFObjectFileTest.cpp
===
--- llvm/unittests/Object/ELFObjectFileTest.cpp
+++ llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -7,8 +7,9 @@
 //===--===//
 
 #include "llvm/Object/ELFObjectFile.h"
-#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
@@ -86,38 +87,33 @@
 TEST(ELFObjectFileTest, MachineTestForNoneOrUnused) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_NONE))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_NONE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 
   // Test an arbitrary unused EM_* value (255).
-  I = 0;
-  for (const DataForTest &D : generateData(255))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(255)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForVE) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-ve", "elf64-ve"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_VE))
-checkFormatAndArch(D, Formats[I++], Triple::ve);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_VE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::ve);
 }
 
 TEST(ELFObjectFileTest, MachineTestForX86_64) {
   std::array Formats = {"elf32-x86-64", "elf32-x86-64",
   "elf64-x86-64", "elf64-x86-64"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_X86_64))
-checkFormatAndArch(D, Formats[I++], Triple::x86_64);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_X86_64)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86_64);
 }
 
 TEST(ELFObjectFileTest, MachineTestFor386) {
   std::array Formats = {"elf32-i386", "elf32-i386", "elf64-i386",
   "elf64-i386"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_386))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_386)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForMIPS) {
@@ -125,27 +121,22 @@
   "elf64-mips"};
   std::array Archs = {Triple::mipsel, Triple::mips,
Triple::mips64el, Triple::mips64};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_MIPS)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_MIPS)))
+checkFormatAndArch(Data, Formats[Idx], Archs[Idx]);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAMDGPU) {
   std::array Formats = {"elf32-amdgpu", "elf32-amdgpu",
   "elf64-amdgpu", "elf64-amdgpu"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AMDGPU))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_AMDGPU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForIAMCU) {
   std::array Formats = {"elf32-iamcu", "elf32-iamcu",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_IAMCU))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_IAMCU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAARCH64) {
@@ -154,11 +145,8 @@
   "elf64-bigaarch64"};
   std::array Archs = {Triple::aarch64, Triple::aarch64_be,
Triple::aarch64, Triple::aarch64_be};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AARCH64)) {
-checkFormatAndArch(D,

[PATCH] D141788: [NFC] Use `llvm::enumerate` in llvm/unittests/Object

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 489341.
barannikov88 added a comment.

Rebase onto main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141788

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/unittests/Object/ELFObjectFileTest.cpp

Index: llvm/unittests/Object/ELFObjectFileTest.cpp
===
--- llvm/unittests/Object/ELFObjectFileTest.cpp
+++ llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -7,8 +7,9 @@
 //===--===//
 
 #include "llvm/Object/ELFObjectFile.h"
-#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
@@ -86,38 +87,33 @@
 TEST(ELFObjectFileTest, MachineTestForNoneOrUnused) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_NONE))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_NONE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 
   // Test an arbitrary unused EM_* value (255).
-  I = 0;
-  for (const DataForTest &D : generateData(255))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(255)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForVE) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-ve", "elf64-ve"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_VE))
-checkFormatAndArch(D, Formats[I++], Triple::ve);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_VE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::ve);
 }
 
 TEST(ELFObjectFileTest, MachineTestForX86_64) {
   std::array Formats = {"elf32-x86-64", "elf32-x86-64",
   "elf64-x86-64", "elf64-x86-64"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_X86_64))
-checkFormatAndArch(D, Formats[I++], Triple::x86_64);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_X86_64)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86_64);
 }
 
 TEST(ELFObjectFileTest, MachineTestFor386) {
   std::array Formats = {"elf32-i386", "elf32-i386", "elf64-i386",
   "elf64-i386"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_386))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_386)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForMIPS) {
@@ -125,27 +121,22 @@
   "elf64-mips"};
   std::array Archs = {Triple::mipsel, Triple::mips,
Triple::mips64el, Triple::mips64};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_MIPS)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_MIPS)))
+checkFormatAndArch(Data, Formats[Idx], Archs[Idx]);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAMDGPU) {
   std::array Formats = {"elf32-amdgpu", "elf32-amdgpu",
   "elf64-amdgpu", "elf64-amdgpu"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AMDGPU))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_AMDGPU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForIAMCU) {
   std::array Formats = {"elf32-iamcu", "elf32-iamcu",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_IAMCU))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_IAMCU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAARCH64) {
@@ -154,11 +145,8 @@
   "elf64-bigaarch64"};
   std::array Archs = {Triple::aarch64, Triple::aarch64_be,
Triple::aarch64, Triple::aarch64_be};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AARCH64)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (const auto &[Idx, Data] : enumerate(generateData(ELF::EM_AARCH64)

[PATCH] D141798: Remove ZeroBehavior of countLeadingZeros and the like (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

It would be nice to have comments reflecting the new behavior in the case of 0 
/ max value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141798

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

[PATCH] D141788: [NFC] Use `llvm::enumerate` in llvm/unittests/Object

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D141788#4055263 , @MaskRay wrote:

> non-reference `auto` shall work better in these cases.

Not sure why? Won't Data be copied every iteration?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141788

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

[PATCH] D141788: [NFC] Use `llvm::enumerate` in llvm/unittests/Object

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 489499.
barannikov88 added a comment.

Use `auto` instead of `const auto &`
Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141788

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/unittests/Object/ELFObjectFileTest.cpp

Index: llvm/unittests/Object/ELFObjectFileTest.cpp
===
--- llvm/unittests/Object/ELFObjectFileTest.cpp
+++ llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -7,8 +7,9 @@
 //===--===//
 
 #include "llvm/Object/ELFObjectFile.h"
-#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
@@ -86,38 +87,33 @@
 TEST(ELFObjectFileTest, MachineTestForNoneOrUnused) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_NONE))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_NONE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 
   // Test an arbitrary unused EM_* value (255).
-  I = 0;
-  for (const DataForTest &D : generateData(255))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(255)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForVE) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-ve", "elf64-ve"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_VE))
-checkFormatAndArch(D, Formats[I++], Triple::ve);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_VE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::ve);
 }
 
 TEST(ELFObjectFileTest, MachineTestForX86_64) {
   std::array Formats = {"elf32-x86-64", "elf32-x86-64",
   "elf64-x86-64", "elf64-x86-64"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_X86_64))
-checkFormatAndArch(D, Formats[I++], Triple::x86_64);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_X86_64)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86_64);
 }
 
 TEST(ELFObjectFileTest, MachineTestFor386) {
   std::array Formats = {"elf32-i386", "elf32-i386", "elf64-i386",
   "elf64-i386"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_386))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_386)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForMIPS) {
@@ -125,27 +121,22 @@
   "elf64-mips"};
   std::array Archs = {Triple::mipsel, Triple::mips,
Triple::mips64el, Triple::mips64};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_MIPS)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_MIPS)))
+checkFormatAndArch(Data, Formats[Idx], Archs[Idx]);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAMDGPU) {
   std::array Formats = {"elf32-amdgpu", "elf32-amdgpu",
   "elf64-amdgpu", "elf64-amdgpu"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AMDGPU))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_AMDGPU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForIAMCU) {
   std::array Formats = {"elf32-iamcu", "elf32-iamcu",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_IAMCU))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_IAMCU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAARCH64) {
@@ -154,11 +145,8 @@
   "elf64-bigaarch64"};
   std::array Archs = {Triple::aarch64, Triple::aarch64_be,
Triple::aarch64, Triple::aarch64_be};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AARCH64)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_AARCH64)))
+checkFormatAndArch(Data, For

[PATCH] D141788: [NFC] Use `llvm::enumerate` in llvm/unittests/Object

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a0e0c226018: [NFC] Use `llvm::enumerate` in 
llvm/unittests/Object (authored by barannikov88).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141788

Files:
  clang/docs/tools/clang-formatted-files.txt
  llvm/unittests/Object/ELFObjectFileTest.cpp

Index: llvm/unittests/Object/ELFObjectFileTest.cpp
===
--- llvm/unittests/Object/ELFObjectFileTest.cpp
+++ llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -7,8 +7,9 @@
 //===--===//
 
 #include "llvm/Object/ELFObjectFile.h"
-#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
@@ -86,38 +87,33 @@
 TEST(ELFObjectFileTest, MachineTestForNoneOrUnused) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_NONE))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_NONE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 
   // Test an arbitrary unused EM_* value (255).
-  I = 0;
-  for (const DataForTest &D : generateData(255))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(255)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForVE) {
   std::array Formats = {"elf32-unknown", "elf32-unknown",
   "elf64-ve", "elf64-ve"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_VE))
-checkFormatAndArch(D, Formats[I++], Triple::ve);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_VE)))
+checkFormatAndArch(Data, Formats[Idx], Triple::ve);
 }
 
 TEST(ELFObjectFileTest, MachineTestForX86_64) {
   std::array Formats = {"elf32-x86-64", "elf32-x86-64",
   "elf64-x86-64", "elf64-x86-64"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_X86_64))
-checkFormatAndArch(D, Formats[I++], Triple::x86_64);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_X86_64)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86_64);
 }
 
 TEST(ELFObjectFileTest, MachineTestFor386) {
   std::array Formats = {"elf32-i386", "elf32-i386", "elf64-i386",
   "elf64-i386"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_386))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_386)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForMIPS) {
@@ -125,27 +121,22 @@
   "elf64-mips"};
   std::array Archs = {Triple::mipsel, Triple::mips,
Triple::mips64el, Triple::mips64};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_MIPS)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);
-++I;
-  }
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_MIPS)))
+checkFormatAndArch(Data, Formats[Idx], Archs[Idx]);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAMDGPU) {
   std::array Formats = {"elf32-amdgpu", "elf32-amdgpu",
   "elf64-amdgpu", "elf64-amdgpu"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AMDGPU))
-checkFormatAndArch(D, Formats[I++], Triple::UnknownArch);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_AMDGPU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::UnknownArch);
 }
 
 TEST(ELFObjectFileTest, MachineTestForIAMCU) {
   std::array Formats = {"elf32-iamcu", "elf32-iamcu",
   "elf64-unknown", "elf64-unknown"};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_IAMCU))
-checkFormatAndArch(D, Formats[I++], Triple::x86);
+  for (auto [Idx, Data] : enumerate(generateData(ELF::EM_IAMCU)))
+checkFormatAndArch(Data, Formats[Idx], Triple::x86);
 }
 
 TEST(ELFObjectFileTest, MachineTestForAARCH64) {
@@ -154,11 +145,8 @@
   "elf64-bigaarch64"};
   std::array Archs = {Triple::aarch64, Triple::aarch64_be,
Triple::aarch64, Triple::aarch64_be};
-  size_t I = 0;
-  for (const DataForTest &D : generateData(ELF::EM_AARCH64)) {
-checkFormatAndArch(D, Formats[I], Archs[I]);

[PATCH] D141581: [clang] Make clangBasic and clangDriver depend on LLVMTargetParser.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D141581#4056503 , @fpetrogalli 
wrote:

> This is because the sources of clangBasic and clangDriver might be compiled 
> before LLVMTargetParser is ready.

...

> Therefore, if we say that clangDriver and clangBasic depend on 
> LLVMTargetParser we make sure that the inclusion of the tablegen-generated 
> file resolves correctly.

Sorry, I don't follow. If I read correctly, you're saying that clang libraries 
might begin to //compile// before their DEPENDS dependency is built (implying 
that DEPENDS clause only guarantees that the dependency is ready at //link// 
stage). If it is true, the proposed patch changes nothing -- the sources might 
still start to compile before cmake decides to generate inc file, because it is 
only needed at link stage.
Am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141581

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

[PATCH] D141581: [clang] Make clangBasic and clangDriver depend on LLVMTargetParser.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D141581#4056671 , @fpetrogalli 
wrote:

> @barannikov88  - I am stuck with an incomplete explanation:

Ah, I see. The key point is that standalone build that depends on //installed// 
LLVM.
RISCVTargetParser is a custom target and it is not being installed, while 
LLVMTargetParser is a "real" target and gets installed. This is probably why 
changing the dependency fixes the issue.
The change looks good to me then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141581

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

[PATCH] D141581: [clang] Make clangBasic and clangDriver depend on LLVMTargetParser.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

clangBasic and clangDriver already have a dependency on TargetParser (see 
LLVM_LINK_COMPONENTS at the beginning of corresponding files). Is that not 
enough?
Will it build if you just remove the additional dependency?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141581

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

[PATCH] D141918: WIP: [Clang] Emit 'unwindabort' when applicable.

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

I couldn't find an example of creating "resume unwindabort" in the patch 
series. Is it yet to be done?
Could you show a source code example where such an instruction should be 
generated? (By the frontend or an IR pass.) I couldn't get it from the RFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141918

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

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/Support/CodeGen.h:57
+  /// Code generation optimization level.
+  enum Level : IDType {
+None = 0,  ///< -O0

arsenm wrote:
> scott.linder wrote:
> > This is ABI breaking, so maybe we don't want/need to define the underlying 
> > type?
> This isn't in the C API, and this isn't for a release branch so I don't think 
> it matters
Why did you need to restrict the underlying type?




Comment at: llvm/include/llvm/Support/CodeGen.h:72
+  /// Get the integer \c ID of \p Level.
+  inline int getID(CodeGenOpt::Level Level) {
+return static_cast(Level);

Should return `IDType`.



Comment at: llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp:44
  cl::desc("Optimization level. [-O0, -O1, -O2, or -O3] "
-  "(default = '-O2')"),
- cl::Prefix, cl::init(' '));
+  "(default = '-O0')"),
+ cl::Prefix, cl::init('2'));

This disagrees with cl::init('2')


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141968

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

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/Support/CodeGen.h:67
+  inline std::optional getLevel(IDType ID) {
+if (ID < 0 || ID > 3)
+  return std::nullopt;

As I can see, clients do not check for nullopt. Either add checks or replace 
this check with an assertion and drop std::optional (in this case `parseLevel` 
should be updated accordingly).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141968

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

[PATCH] D141968: [NFC] Consolidate llvm::CodeGenOpt::Level handling

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/include/llvm/Support/CodeGen.h:57
+  /// Code generation optimization level.
+  enum Level : IDType {
+None = 0,  ///< -O0

scott.linder wrote:
> barannikov88 wrote:
> > arsenm wrote:
> > > scott.linder wrote:
> > > > This is ABI breaking, so maybe we don't want/need to define the 
> > > > underlying type?
> > > This isn't in the C API, and this isn't for a release branch so I don't 
> > > think it matters
> > Why did you need to restrict the underlying type?
> > 
> There is no strict need, it just:
> 
> * Avoids potential UB (casting an out-of-range value to an enumeration type 
> //without a fixed underlying type// is undefined); in practice there is 
> plenty of UB in LLVM, and a bug here wouldn't be a particularly pernicious 
> kind of UB anyway.
> * Means users of the type might pack better (and we won't run out of 255 opt 
> levels anytime soon).
> 
> I originally intended to change this to an `enum class` rather than a `class` 
> in a `namespace`, but that is slightly more disruptive and should probably be 
> done for every other enumeration defined here at the same time.
Thanks for the explanation.
> Means users of the type might pack better (and we won't run out of 255 opt 
> levels anytime soon).
The downside is that `int` is usually more efficient.
> Avoids potential UB [...]
cppreference [[ https://en.cppreference.com/w/cpp/language/enum | says ]]
```
Values of unscoped enumeration type are implicitly-convertible to integral 
types. If the underlying type is not fixed, the value is convertible to the 
first type from the following list able to hold their entire value range: int, 
unsigned int, ...
```
so casting to int should be safe.
Anyways, this is just a nit, feel free to ignore.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141968

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

[PATCH] D141798: Drop the ZeroBehavior parameter from countLeadingZeros and the like (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

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

LGTM




Comment at: llvm/include/llvm/Support/MathExtras.h:212
 /// Only unsigned integral types are allowed.
-///
-/// \param ZB the behavior on an input of 0. Only ZB_Max and ZB_Undefined are
-///   valid arguments.
-template  T findFirstSet(T Val, ZeroBehavior ZB = ZB_Max) {
-  if (ZB == ZB_Max && Val == 0)
+template  T findFirstSet(T Val) {
+  if (Val == 0)

kazu wrote:
> craig.topper wrote:
> > Note, x86 does not have an efficient instruction for find first set with 
> > zero returning -1. It will require a cmov to handle zero.
> The new iteration of the patch leaves findFirstSet and findLastSet untouched.
(optional) ZB could be made a template argument and constexpr-ifed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141798

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

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 557642.
barannikov88 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

Files:
  clang/test/Preprocessor/has_attribute.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3434,9 +3434,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3455,26 +3456,22 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
+assert(Spelling.variety() == Variety);
 std::string Name = "";
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Name = Spelling.name();
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Name = Spelling.name();
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3514,10 +3511,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variety.empty() || (Variety == S.variety() &&
-  (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+if (Scope.empty() || Scope == Spelling.nameSpace())
+  OS << ".Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << ".Default(0);\n";
 }
@@ -3550,8 +3545,11 @@
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map> CXX, C23;
+  std::vector> Declspec, Microsoft,
+  GNU, Pragma, HLSLSemantic;
+  std::map>>
+  CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -3560,19 +3558,19 @@
 for (const auto &SI : Spellings) {
   const std::string &Variety = SI.variety();
   if (Variety == "GNU")
-GNU.push_back(R);
+GNU.emplace_back(R, SI);
   else if (Variety == "Declspec")
-Declspec.push_back(R);
+Declspec.emplace_back(R, SI);
   else if (Variety == "Microsoft")
-Microsoft.push_back(R);
+Microsoft.emplace_back(R, SI);
   else if (Variety == "CXX11")
-CXX[SI.nameSpace()].push_back(R);
+CXX[SI.nameSpace()].emplace_back(R, SI);
   else if (Variety == "C23")
-C23[SI.nameS

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 557643.
barannikov88 added a comment.

Add a release note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

Files:
  clang/docs/ReleaseNotes.rst
  clang/test/Preprocessor/has_attribute.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3434,9 +3434,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3455,26 +3456,22 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
+assert(Spelling.variety() == Variety);
 std::string Name = "";
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Name = Spelling.name();
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Name = Spelling.name();
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3514,10 +3511,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variety.empty() || (Variety == S.variety() &&
-  (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+if (Scope.empty() || Scope == Spelling.nameSpace())
+  OS << ".Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << ".Default(0);\n";
 }
@@ -3550,8 +3545,11 @@
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map> CXX, C23;
+  std::vector> Declspec, Microsoft,
+  GNU, Pragma, HLSLSemantic;
+  std::map>>
+  CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -3560,19 +3558,19 @@
 for (const auto &SI : Spellings) {
   const std::string &Variety = SI.variety();
   if (Variety == "GNU")
-GNU.push_back(R);
+GNU.emplace_back(R, SI);
   else if (Variety == "Declspec")
-Declspec.push_back(R);
+Declspec.emplace_back(R, SI);
   else if (Variety == "Microsoft")
-Microsoft.push_back(R);
+Microsoft.emplace_back(R, SI);
   else if (Variety == "CXX11")
-CXX[SI.nameSpace()].push_back(R);
+CXX[SI.nameSpace()].emplace_back(R, SI);
   else i

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

@aaron.ballman I added a release note as requested. Please see if it looks the 
way it should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

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

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 557644.
barannikov88 added a comment.

Add a hyphen


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

Files:
  clang/docs/ReleaseNotes.rst
  clang/test/Preprocessor/has_attribute.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3434,9 +3434,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3455,26 +3456,22 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
+assert(Spelling.variety() == Variety);
 std::string Name = "";
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Name = Spelling.name();
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Name = Spelling.name();
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3514,10 +3511,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variety.empty() || (Variety == S.variety() &&
-  (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+if (Scope.empty() || Scope == Spelling.nameSpace())
+  OS << ".Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << ".Default(0);\n";
 }
@@ -3550,8 +3545,11 @@
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map> CXX, C23;
+  std::vector> Declspec, Microsoft,
+  GNU, Pragma, HLSLSemantic;
+  std::map>>
+  CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -3560,19 +3558,19 @@
 for (const auto &SI : Spellings) {
   const std::string &Variety = SI.variety();
   if (Variety == "GNU")
-GNU.push_back(R);
+GNU.emplace_back(R, SI);
   else if (Variety == "Declspec")
-Declspec.push_back(R);
+Declspec.emplace_back(R, SI);
   else if (Variety == "Microsoft")
-Microsoft.push_back(R);
+Microsoft.emplace_back(R, SI);
   else if (Variety == "CXX11")
-CXX[SI.nameSpace()].push_back(R);
+CXX[SI.nameSpace()].emplace_back(R, SI);
   else if (Var

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG79f87be6888d: [clang] Fix several issues in the generated 
AttrHasAttributeImpl.inc (authored by barannikov88).

Changed prior to commit:
  https://reviews.llvm.org/D159393?vs=557644&id=557657#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

Files:
  clang/docs/ReleaseNotes.rst
  clang/test/Preprocessor/has_attribute.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3434,9 +3434,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3455,26 +3456,22 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
+assert(Spelling.variety() == Variety);
 std::string Name = "";
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Name = Spelling.name();
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Name = Spelling.name();
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3514,10 +3511,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variety.empty() || (Variety == S.variety() &&
-  (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+if (Scope.empty() || Scope == Spelling.nameSpace())
+  OS << ".Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << ".Default(0);\n";
 }
@@ -3550,8 +3545,11 @@
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map> CXX, C23;
+  std::vector> Declspec, Microsoft,
+  GNU, Pragma, HLSLSemantic;
+  std::map>>
+  CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -3560,19 +3558,19 @@
 for (const auto &SI : Spellings) {
   const std::string &Variety = SI.variety();
   if (Variety == "GNU")
-GNU.push_back(R);
+GNU.emplace_back(R, SI);
   else if (Variety == "Declspec")
-Declspec.push_back(R);
+Declspec.emplace_back(R, SI);
   else if (Variety == "Microsoft")
-Microsoft.push_bac

[PATCH] D156237: Complete the implementation of P2361 Unevaluated string literals

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2349
+  if (isVariadicStringLiteralArgument(Args[N])) {
+for (; N < sizeof(uint32_t); N++)
+  Bits |= (1 << N);

maskTrailingZeros might also be useful


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156237

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 accepted this revision.
barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3954
 Function *F = CGM.getIntrinsic(Intrinsic::eh_sjlj_setjmp);
-Buf = Builder.CreateElementBitCast(Buf, Int8Ty);
 return RValue::get(Builder.CreateCall(F, Buf.getPointer()));

jrtc27 wrote:
> Missed `Buf = Buf.withElementType(Int8Ty);`? According to the comment above, 
> Buf is a `void **` not a `void *`/`char *`.
It would be a dead code because only the pointer is used down below.




Comment at: clang/lib/CodeGen/CGExpr.cpp:3896
 llvm::Type *OrigBaseElemTy = Addr.getElementType();
-Addr = Builder.CreateElementBitCast(Addr, Int8Ty);
 

jrtc27 wrote:
> This one isn't a direct substitution, although appears to be refactoring the 
> code to not need withElementType in the first place? Probably best to 
> separate that change out from the very mechanical substitution.
It will be difficult to find all such places later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/Targets/AMDGPU.cpp:520
+static llvm::StructType *getAMDGPUKernelDescriptorType(llvm::LLVMContext &C) {
+  llvm::Type *Int8 = llvm::IntegerType::getInt8Ty(C);
+  llvm::Type *Int16 = llvm::IntegerType::getInt16Ty(C);

Minor suggestion: you can get these types from CGF / CGM (Int8Ty etc.)



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

https://reviews.llvm.org/D141700

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

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:138
 - Implemented `P2738R1: constexpr cast from void* 
`_.
+- Partially implemented `P2361R6: constexpr cast from void* 
`_.
+  The changes to attributes declarations are not part of this release.

Looks like a copy&paste bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759

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

[PATCH] D105759: Implement P2361 Unevaluated string literals

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

@cor3ntin 
I've been working on pretty much the same functionality in our downstream fork. 
I was not aware of the paper, nor of the ongoing work in this direction, and so 
I unfortunately missed the review.
Thanks for this patch, it significantly reduces the number of changes 
downstream and makes it easier to merge with upstream in the future.

I have a couple of questions about future work:

- IIUC the paper initially addressed this issue with `#line` directive, but the 
changes were reverted(?). Is there any chance they can get back?
- Are there any plans for making similar changes to asm statement parsing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105759

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

[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3883
+  ///< message.
+CCEK_StaticAssertMessageData, ///< Call to data() in a static assert
+  ///< message.

Appears unused.



Comment at: clang/lib/AST/ExprConstant.cpp:16413
+APSInt C = Char.getInt();
+Result.push_back(static_cast(C.getExtValue()));
+if (!HandleLValueArrayAdjustment(Info, PtrExpression, String, CharTy, 1))

This relies on host's CHAR_BIT >= target's CHAR_BIT, which isn't true for my 
target. Could you add an assertion?




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16960
+  ExprResult EvaluatedData = BuildConvertedConstantExpression(
+  DataE.get(), ConstCharPtr, CCEK_StaticAssertMessageSize);
+  if (EvaluatedData.isInvalid()) {





Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:127
+};
+static_assert(false, RAII{}); // expected-error {{static assertion failed: ok}}

Should there be (negative?) tests with non-const data/size members and 
incorrect number of parameters? These conditions are checked in FindMember.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290

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

[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

According to the current wording, the static_assert-message is either 
unevaluated string or an expression evaluated at compile time.
Unevaluated strings don't allow certain escape sequences, but if I wrap the 
string in a string_view-like class, I'm allowed to use any escape sequeces, 
including '\x'.
Moreover, wrapping a string in a class would change its encoding. Unevaluated 
strings are displayed as written in the source (that is, UTF-8), while wrapped 
strings undergo conversion to execution encoding (e.g. EBCDIC) and then printed 
in system locale, leading to mojibake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290

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

[PATCH] D154773: [AST] Use correct APSInt width when evaluating string literals

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
barannikov88 added a reviewer: cor3ntin.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The width of the APSInt values should be the width of an element.
getCharByteWidth returns the size of an element in _host_ bytes, which
makes the width N times greater, where N is the ratio between target's
CHAR_BIT and host's CHAR_BIT.
This is NFC for in-tree targets because all of them have CHAR_BIT == 8.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154773

Files:
  clang/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3420,8 +3420,7 @@
   assert(CAT && "string literal isn't an array");
   QualType CharType = CAT->getElementType();
   assert(CharType->isIntegerType() && "unexpected character type");
-
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Index < S->getLength())
 Value = S->getCodeUnit(Index);
@@ -3444,7 +3443,7 @@
   unsigned Elts = CAT->getSize().getZExtValue();
   Result = APValue(APValue::UninitArray(),
std::min(S->getLength(), Elts), Elts);
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Result.hasArrayFiller())
 Result.getArrayFiller() = APValue(Value);


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3420,8 +3420,7 @@
   assert(CAT && "string literal isn't an array");
   QualType CharType = CAT->getElementType();
   assert(CharType->isIntegerType() && "unexpected character type");
-
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Index < S->getLength())
 Value = S->getCodeUnit(Index);
@@ -3444,7 +3443,7 @@
   unsigned Elts = CAT->getSize().getZExtValue();
   Result = APValue(APValue::UninitArray(),
std::min(S->getLength(), Elts), Elts);
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Result.hasArrayFiller())
 Result.getArrayFiller() = APValue(Value);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D154290#4483055 , @cor3ntin wrote:

> In D154290#4482975 , @barannikov88 
> wrote:
>
>> According to the current wording, the static_assert-message is either 
>> unevaluated string or an expression evaluated at compile time.
>> Unevaluated strings don't allow certain escape sequences, but if I wrap the 
>> string in a string_view-like class, I'm allowed to use any escape sequeces, 
>> including '\x'.
>> Moreover, wrapping a string in a class would change its encoding. 
>> Unevaluated strings are displayed as written in the source (that is, UTF-8), 
>> while wrapped strings undergo conversion to execution encoding (e.g. EBCDIC) 
>> and then printed in system locale, leading to mojibake.
>
> Not quite.
> Unevaluated strings are always UTF-8 ( regardless of source file encoding). 
> Evaluated strings are in the literal encoding which is always UTF-8 for 
> clang. 
> This will change whenever we allow for different kinds of literal encodings 
> per  this RFC 
> https://discourse.llvm.org/t/rfc-enabling-fexec-charset-support-to-llvm-and-clang-reposting/71512/1
>
> If and when that is the case we will have to convert back to UTF-8 before 
> displaying - and then maybe convert back to the system locale depending on 
> host.
> Numeric escape sequences can then occur in evaluated strings and produce 
> mojibake if the evaluated strings is not valid in the string literal encoding.
> I don't believe that we would want to output static messages without 
> conversion on any system as the diagnostics framework is very much geared 
> towards UTF-8 and we want to keep supporting cross compilation.
>
> So the process will be
> source -> utf8 -> literal encoding -> utf8 -> terminal encoding.

Thanks for your reply, I think I see the idea.

> By the same account, casting 0-extended utf-8 to char is fine until such time 
> clang support more than UTF-8. (which is one of the reasons we need to make 
> sure clang conversions utilities can convert from and to utf-8)
>
> Unevaluated strings were introduced in part to help identify what gets 
> converted and what does not.

It is a bit strange that the string in `static_assert(false, "й")` is not 
converted, while it is converted in `static_assert(false, 
std::string_view("й"))`.
It might be possible to achieve identical diagnostic output even with 
-fexec-charset supported (which would only affect the second form),
but right now I'm confused by the distinction… Why don't always evaluate the 
message?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290

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

[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:16413
+APSInt C = Char.getInt();
+Result.push_back(static_cast(C.getExtValue()));
+if (!HandleLValueArrayAdjustment(Info, PtrExpression, String, CharTy, 1))

aaron.ballman wrote:
> barannikov88 wrote:
> > This relies on host's CHAR_BIT >= target's CHAR_BIT, which isn't true for 
> > my target. Could you add an assertion?
> > 
> Wouldn't adding the assertion cause you problems then? (FWIW, we only support 
> `CHAR_BIT == 8` currently.)
It will, the assertion will help find this place.
There are several places where it is asserted, and this was very helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154290

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

[PATCH] D159024: [Parser] Parse string literal arguments of 'availability', 'external_source_symbol' and 'uuid' attributes as unevaluated

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
barannikov88 added reviewers: cor3ntin, aaron.ballman.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a complementary to D156237 .
These attributes have custom parsing logic.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159024

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/attr-availability.c
  clang/test/Parser/attr-external-source-symbol.m
  clang/test/Parser/ms-square-bracket-attributes.mm

Index: clang/test/Parser/ms-square-bracket-attributes.mm
===
--- clang/test/Parser/ms-square-bracket-attributes.mm
+++ clang/test/Parser/ms-square-bracket-attributes.mm
@@ -17,9 +17,9 @@
 )] struct struct_with_uuid_brace;
 
 // uuids must be ascii string literals.
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'u8' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(u8"00A0---C000-0049")] struct struct_with_uuid_u8;
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'L' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(L"00A0---C000-0049")] struct struct_with_uuid_L;
 
 // cl.exe doesn't allow raw string literals in []-style attributes, but does
Index: clang/test/Parser/attr-external-source-symbol.m
===
--- clang/test/Parser/attr-external-source-symbol.m
+++ clang/test/Parser/attr-external-source-symbol.m
@@ -95,6 +95,27 @@
 void f28(void)
 __attribute__((external_source_symbol(USR="")));
 
+void f29(void)
+__attribute__((external_source_symbol(language=L"Swift"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f30(void)
+__attribute__((external_source_symbol(language="Swift", language=L"Swift"))); // expected-error {{duplicate 'language' clause in an 'external_source_symbol' attribute}} \
+  // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f31(void)
+__attribute__((external_source_symbol(USR=u"foo"))); // expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f32(void)
+__attribute__((external_source_symbol(USR="foo", USR=u"foo"))); // expected-error {{duplicate 'USR' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f33(void)
+__attribute__((external_source_symbol(defined_in=U"module"))); // expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
+void f34(void)
+__attribute__((external_source_symbol(defined_in="module", defined_in=U"module"))); // expected-error {{duplicate 'defined_in' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
 #if __has_attribute(external_source_symbol) != 20230206
 # error "invalid __has_attribute version"
 #endif
Index: clang/test/Parser/attr-availability.c
===
--- clang/test/Parser/attr-availability.c
+++ clang/test/Parser/attr-availability.c
@@ -18,17 +18,17 @@
 
 void f6(void) __attribute__((availability(macosx,unavailable,introduced=10.5))); // expected-warning{{'unavailable' availability overrides all other availability information}}
 
-void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-warning {{encoding prefix 'u8' on an unevaluated string literal has no effec

[PATCH] D159024: [Parser] Parse string literal arguments of 'availability', 'external_source_symbol' and 'uuid' attributes as unevaluated

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 554497.
barannikov88 added a comment.

Update the failing test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159024

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/attr-availability-xcore.c
  clang/test/Parser/attr-availability.c
  clang/test/Parser/attr-external-source-symbol.m
  clang/test/Parser/ms-square-bracket-attributes.mm

Index: clang/test/Parser/ms-square-bracket-attributes.mm
===
--- clang/test/Parser/ms-square-bracket-attributes.mm
+++ clang/test/Parser/ms-square-bracket-attributes.mm
@@ -17,9 +17,9 @@
 )] struct struct_with_uuid_brace;
 
 // uuids must be ascii string literals.
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'u8' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(u8"00A0---C000-0049")] struct struct_with_uuid_u8;
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'L' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(L"00A0---C000-0049")] struct struct_with_uuid_L;
 
 // cl.exe doesn't allow raw string literals in []-style attributes, but does
Index: clang/test/Parser/attr-external-source-symbol.m
===
--- clang/test/Parser/attr-external-source-symbol.m
+++ clang/test/Parser/attr-external-source-symbol.m
@@ -95,6 +95,27 @@
 void f28(void)
 __attribute__((external_source_symbol(USR="")));
 
+void f29(void)
+__attribute__((external_source_symbol(language=L"Swift"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f30(void)
+__attribute__((external_source_symbol(language="Swift", language=L"Swift"))); // expected-error {{duplicate 'language' clause in an 'external_source_symbol' attribute}} \
+  // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f31(void)
+__attribute__((external_source_symbol(USR=u"foo"))); // expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f32(void)
+__attribute__((external_source_symbol(USR="foo", USR=u"foo"))); // expected-error {{duplicate 'USR' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f33(void)
+__attribute__((external_source_symbol(defined_in=U"module"))); // expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
+void f34(void)
+__attribute__((external_source_symbol(defined_in="module", defined_in=U"module"))); // expected-error {{duplicate 'defined_in' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
 #if __has_attribute(external_source_symbol) != 20230206
 # error "invalid __has_attribute version"
 #endif
Index: clang/test/Parser/attr-availability.c
===
--- clang/test/Parser/attr-availability.c
+++ clang/test/Parser/attr-availability.c
@@ -18,17 +18,17 @@
 
 void f6(void) __attribute__((availability(macosx,unavailable,introduced=10.5))); // expected-warning{{'unavailable' availability overrides all other availability information}}
 
-void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-warning {{encoding prefix 'u8' on an unevaluated string literal has no effect}}
 
-void f10(void) __attribute__((availability(macosx,message="a" u8"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}

[PATCH] D159024: [Parser] Parse string literal arguments of 'availability', 'external_source_symbol' and 'uuid' attributes as unevaluated

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7eaaba69906: [Parser] Parse string literal arguments of 
'availability'… (authored by barannikov88).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159024

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/attr-availability-xcore.c
  clang/test/Parser/attr-availability.c
  clang/test/Parser/attr-external-source-symbol.m
  clang/test/Parser/ms-square-bracket-attributes.mm

Index: clang/test/Parser/ms-square-bracket-attributes.mm
===
--- clang/test/Parser/ms-square-bracket-attributes.mm
+++ clang/test/Parser/ms-square-bracket-attributes.mm
@@ -17,9 +17,9 @@
 )] struct struct_with_uuid_brace;
 
 // uuids must be ascii string literals.
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'u8' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(u8"00A0---C000-0049")] struct struct_with_uuid_u8;
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-warning@+1 {{encoding prefix 'L' on an unevaluated string literal has no effect and is incompatible with c++2c}}
 [uuid(L"00A0---C000-0049")] struct struct_with_uuid_L;
 
 // cl.exe doesn't allow raw string literals in []-style attributes, but does
Index: clang/test/Parser/attr-external-source-symbol.m
===
--- clang/test/Parser/attr-external-source-symbol.m
+++ clang/test/Parser/attr-external-source-symbol.m
@@ -95,6 +95,27 @@
 void f28(void)
 __attribute__((external_source_symbol(USR="")));
 
+void f29(void)
+__attribute__((external_source_symbol(language=L"Swift"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f30(void)
+__attribute__((external_source_symbol(language="Swift", language=L"Swift"))); // expected-error {{duplicate 'language' clause in an 'external_source_symbol' attribute}} \
+  // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
+
+void f31(void)
+__attribute__((external_source_symbol(USR=u"foo"))); // expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f32(void)
+__attribute__((external_source_symbol(USR="foo", USR=u"foo"))); // expected-error {{duplicate 'USR' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'u' on an unevaluated string literal has no effect}}
+
+void f33(void)
+__attribute__((external_source_symbol(defined_in=U"module"))); // expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
+void f34(void)
+__attribute__((external_source_symbol(defined_in="module", defined_in=U"module"))); // expected-error {{duplicate 'defined_in' clause in an 'external_source_symbol' attribute}} \
+// expected-warning {{encoding prefix 'U' on an unevaluated string literal has no effect}}
+
 #if __has_attribute(external_source_symbol) != 20230206
 # error "invalid __has_attribute version"
 #endif
Index: clang/test/Parser/attr-availability.c
===
--- clang/test/Parser/attr-availability.c
+++ clang/test/Parser/attr-availability.c
@@ -18,17 +18,17 @@
 
 void f6(void) __attribute__((availability(macosx,unavailable,introduced=10.5))); // expected-warning{{'unavailable' availability overrides all other availability information}}
 
-void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f7(void) __attribute__((availability(macosx,message=L"wide"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f8(void) __attribute__((availability(macosx,message="a" L"b"))); // expected-warning {{encoding prefix 'L' on an unevaluated string literal has no effect}}
 
-void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-error {{expected string literal for optional message in 'availability' attribute}}
+void f9(void) __attribute__((availability(macosx,message=u8"b"))); // expected-warning {{encoding prefix 'u8' on an unevaluated string literal has no effect}}
 
-void f10(void) __attribute__((availability(macosx,message="a" u8"b"))); // expe

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 created this revision.
Herald added subscribers: s.egerton, PkmX, simoncook, kristof.beyls, 
krytarowski, arichardson, dylanmckay.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
barannikov88 requested review of this revision.
Herald added subscribers: cfe-commits, wangpc.
Herald added a project: clang.

1. The generated file contained a lot of duplicate switch cases, e.g.:

  switch (Syntax) {
  case AttributeCommonInfo::Syntax::AS_GNU:
return llvm::StringSwitch(Name)
  ...
  .Case("error", 1)
  .Case("warning", 1)
  .Case("error", 1)
  .Case("warning", 1)



2. Some attributes were listed in wrong places, e.g.:

  case AttributeCommonInfo::Syntax::AS_CXX11: {
  if (ScopeName == "") {
return llvm::StringSwitch(Name)
  ...
  .Case("warn_unused_result", LangOpts.CPlusPlus11 ? 201907 : 0)

`warn_unused_result` is a non-standard attribute and should not be
available as [[warn_unused_result]].

3. Some attributes had the wrong version, e.g.:

  case AttributeCommonInfo::Syntax::AS_CXX11: {
  } else if (ScopeName == "gnu") {
return llvm::StringSwitch(Name)
  ...
  .Case("fallthrough", LangOpts.CPlusPlus11 ? 201603 : 0)

[[gnu::fallthrough]] is a non-standard spelling and should not have the
standard version. Instead, __has_cpp_attribute should return 1 for it.

There is another issue with attributes that share spellings, e.g.:

  .Case("interrupt", true && (T.getArch() == llvm::Triple::arm || ...) ? 1 
: 0)
  .Case("interrupt", true && (T.getArch() == llvm::Triple::avr) ? 1 : 0)
  ...
  .Case("interrupt", true && (T.getArch() == llvm::Triple::riscv32 || ...) 
? 1 : 0)

As can be seen, __has_attribute(interrupt) would only return true for
ARM targets. This patch does not address this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159393

Files:
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3388,9 +3388,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3409,24 +3410,20 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+assert(Spelling.variety() == Variety);
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3446,10 +3443,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variet

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 updated this revision to Diff 51.
barannikov88 added a comment.

Update one more test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

Files:
  clang/test/Preprocessor/has_attribute.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Preprocessor/has_c_attribute.c
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3388,9 +3388,10 @@
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-const std::vector &Attrs, raw_ostream &OS,
-const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+const std::vector> &Attrs,
+raw_ostream &OS, const std::string &Variety,
+const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
 // have the default value (1), as that's incorrect. Instead, the unscoped
@@ -3409,24 +3410,20 @@
 // a way that is impactful to the end user.
 int Version = 1;
 
-std::vector Spellings = GetFlattenedSpellings(*Attr);
-for (const auto &Spelling : Spellings) {
-  if (Spelling.variety() == Variety &&
-  (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-Version = static_cast(
-Spelling.getSpellingRecord().getValueAsInt("Version"));
-// Verify that explicitly specified CXX11 and C23 spellings (i.e.
-// not inferred from Clang/GCC spellings) have a version that's
-// different than the default (1).
-bool RequiresValidVersion =
-(Variety == "CXX11" || Variety == "C23") &&
-Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-if (RequiresValidVersion && Scope.empty() && Version == 1)
-  PrintError(Spelling.getSpellingRecord().getLoc(),
- "Standard attributes must have "
- "valid version information.");
-break;
-  }
+assert(Spelling.variety() == Variety);
+if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+  Version = static_cast(
+  Spelling.getSpellingRecord().getValueAsInt("Version"));
+  // Verify that explicitly specified CXX11 and C23 spellings (i.e.
+  // not inferred from Clang/GCC spellings) have a version that's
+  // different from the default (1).
+  bool RequiresValidVersion =
+  (Variety == "CXX11" || Variety == "C23") &&
+  Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
+  if (RequiresValidVersion && Scope.empty() && Version == 1)
+PrintError(Spelling.getSpellingRecord().getLoc(),
+   "Standard attributes must have "
+   "valid version information.");
 }
 
 std::string Test;
@@ -3446,10 +3443,8 @@
 std::string TestStr = !Test.empty()
   ? Test + " ? " + llvm::itostr(Version) + " : 0"
   : llvm::itostr(Version);
-for (const auto &S : Spellings)
-  if (Variety.empty() || (Variety == S.variety() &&
-  (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+if (Scope.empty() || Scope == Spelling.nameSpace())
+  OS << ".Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << ".Default(0);\n";
 }
@@ -3481,8 +3476,11 @@
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map> CXX, C23;
+  std::vector> Declspec, Microsoft,
+  GNU, Pragma, HLSLSemantic;
+  std::map>>
+  CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -3491,19 +3489,19 @@
 for (const auto &SI : Spellings) {
   const std::string &Variety = SI.variety();
   if (Variety == "GNU")
-GNU.push_back(R);
+GNU.emplace_back(R, SI);
   else if (Variety == "Declspec")
-Declspec.push_back(R);
+Declspec.emplace_back(R, SI);
   else if (Variety == "Microsoft")
-Microsoft.push_back(R);
+Microsoft.emplace_back(R, SI);
   else if (Variety == "CXX11")
-CXX[SI.nameSpace()].push_back(R);
+CXX[SI.nameSpace()].emplace_back(R, SI);
   else if (Variety == "C23")
-C23[SI.nameSpace()].push_back(R);
+C23[SI.nameSpace()].emplace_back(R, SI);
  

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/test/Preprocessor/has_attribute.cpp:35
+CXX11(clang::warn_unused_result)
+
 // CHECK: __gnu__::__const__: 1

For the context, the attribute is defined with the following spellings:
```
  let Spellings = [CXX11<"", "nodiscard", 201907>,
   C23<"", "nodiscard", 202003>,
   CXX11<"clang", "warn_unused_result">,
   GCC<"warn_unused_result">];
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

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

[PATCH] D154773: [AST] Use correct APSInt width when evaluating string literals

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG18a628ec4ef7: [AST] Use correct APSInt width when evaluating 
string literals (authored by barannikov88).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154773

Files:
  clang/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3463,8 +3463,7 @@
   assert(CAT && "string literal isn't an array");
   QualType CharType = CAT->getElementType();
   assert(CharType->isIntegerType() && "unexpected character type");
-
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Index < S->getLength())
 Value = S->getCodeUnit(Index);
@@ -3487,7 +3486,7 @@
   unsigned Elts = CAT->getSize().getZExtValue();
   Result = APValue(APValue::UninitArray(),
std::min(S->getLength(), Elts), Elts);
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Result.hasArrayFiller())
 Result.getArrayFiller() = APValue(Value);


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -3463,8 +3463,7 @@
   assert(CAT && "string literal isn't an array");
   QualType CharType = CAT->getElementType();
   assert(CharType->isIntegerType() && "unexpected character type");
-
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Index < S->getLength())
 Value = S->getCodeUnit(Index);
@@ -3487,7 +3486,7 @@
   unsigned Elts = CAT->getSize().getZExtValue();
   Result = APValue(APValue::UninitArray(),
std::min(S->getLength(), Elts), Elts);
-  APSInt Value(S->getCharByteWidth() * Info.Ctx.getCharWidth(),
+  APSInt Value(Info.Ctx.getTypeSize(CharType),
CharType->isUnsignedIntegerType());
   if (Result.hasArrayFiller())
 Result.getArrayFiller() = APValue(Value);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159393: [clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Should I create a github PR instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159393

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

[PATCH] D153196: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG44e63ffe2bf7: [clang] Replace uses of 
CGBuilderTy::CreateElementBitCast (NFC) (authored by JOE1994, committed by 
barannikov88).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153196

Files:
  clang/lib/CodeGen/Address.h
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuilder.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/CodeGen/Targets/Hexagon.cpp
  clang/lib/CodeGen/Targets/Sparc.cpp
  clang/lib/CodeGen/Targets/SystemZ.cpp

Index: clang/lib/CodeGen/Targets/SystemZ.cpp
===
--- clang/lib/CodeGen/Targets/SystemZ.cpp
+++ clang/lib/CodeGen/Targets/SystemZ.cpp
@@ -302,8 +302,7 @@
 Address OverflowArgArea =
 Address(CGF.Builder.CreateLoad(OverflowArgAreaPtr, "overflow_arg_area"),
 CGF.Int8Ty, TyInfo.Align);
-Address MemAddr =
-CGF.Builder.CreateElementBitCast(OverflowArgArea, DirectTy, "mem_addr");
+Address MemAddr = OverflowArgArea.withElementType(DirectTy);
 
 // Update overflow_arg_area_ptr pointer
 llvm::Value *NewOverflowArgArea = CGF.Builder.CreateGEP(
@@ -360,8 +359,7 @@
   Address RawRegAddr(
   CGF.Builder.CreateGEP(CGF.Int8Ty, RegSaveArea, RegOffset, "raw_reg_addr"),
   CGF.Int8Ty, PaddedSize);
-  Address RegAddr =
-  CGF.Builder.CreateElementBitCast(RawRegAddr, DirectTy, "reg_addr");
+  Address RegAddr = RawRegAddr.withElementType(DirectTy);
 
   // Update the register count
   llvm::Value *One = llvm::ConstantInt::get(IndexTy, 1);
@@ -381,8 +379,7 @@
   CGF.Int8Ty, PaddedSize);
   Address RawMemAddr =
   CGF.Builder.CreateConstByteGEP(OverflowArgArea, Padding, "raw_mem_addr");
-  Address MemAddr =
-CGF.Builder.CreateElementBitCast(RawMemAddr, DirectTy, "mem_addr");
+  Address MemAddr = RawMemAddr.withElementType(DirectTy);
 
   // Update overflow_arg_area_ptr pointer
   llvm::Value *NewOverflowArgArea =
Index: clang/lib/CodeGen/Targets/Sparc.cpp
===
--- clang/lib/CodeGen/Targets/Sparc.cpp
+++ clang/lib/CodeGen/Targets/Sparc.cpp
@@ -315,7 +315,7 @@
   case ABIArgInfo::Indirect:
   case ABIArgInfo::IndirectAliased:
 Stride = SlotSize;
-ArgAddr = Builder.CreateElementBitCast(Addr, ArgPtrTy, "indirect");
+ArgAddr = Addr.withElementType(ArgPtrTy);
 ArgAddr = Address(Builder.CreateLoad(ArgAddr, "indirect.arg"), ArgTy,
   TypeInfo.Align);
 break;
@@ -328,7 +328,7 @@
   Address NextPtr = Builder.CreateConstInBoundsByteGEP(Addr, Stride, "ap.next");
   Builder.CreateStore(NextPtr.getPointer(), VAListAddr);
 
-  return Builder.CreateElementBitCast(ArgAddr, ArgTy, "arg.addr");
+  return ArgAddr.withElementType(ArgTy);
 }
 
 void SparcV9ABIInfo::computeInfo(CGFunctionInfo &FI) const {
Index: clang/lib/CodeGen/Targets/Hexagon.cpp
===
--- clang/lib/CodeGen/Targets/Hexagon.cpp
+++ clang/lib/CodeGen/Targets/Hexagon.cpp
@@ -236,7 +236,7 @@
   // FIXME: Need to handle alignment
   llvm::Type *BP = CGF.Int8PtrTy;
   CGBuilderTy &Builder = CGF.Builder;
-  Address VAListAddrAsBPP = Builder.CreateElementBitCast(VAListAddr, BP, "ap");
+  Address VAListAddrAsBPP = VAListAddr.withElementType(BP);
   llvm::Value *Addr = Builder.CreateLoad(VAListAddrAsBPP, "ap.cur");
   // Handle address alignment for type alignment > 32 bits
   uint64_t TyAlign = CGF.getContext().getTypeAlign(Ty) / 8;
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1279,7 +1279,7 @@
 void MicrosoftCXXABI::EmitVBPtrStores(CodeGenFunction &CGF,
   const CXXRecordDecl *RD) {
   Address This = getThisAddress(CGF);
-  This = CGF.Builder.CreateElementBitCast(This, CGM.Int8Ty, "this.int8");
+  This = This.withElementType(CGM.Int8Ty);
   const ASTContext &Context = getContext();
   const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
 
@@ -1296,8 +1296,7 @@
 Address VBPtr = CGF.Builder.CreateConstInBoundsByteGEP(This, Offs);
 llvm::Value *GVPtr =
 CGF.Builder.CreateConstInBoundsGEP2_32(GV->getValueType(), GV, 0, 0);
-VBPtr = CGF.Builder.CreateElementBitCast(VBPtr, GVPtr->getType(),
-  "vbptr." + VBT->ObjectWithVPtr->getName());
+VBPtr = VBPtr.withElementType(GVPtr->getType())

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

- I'd prefer Lex() over equivalent getParser().Lex() because it is shorter.
- Ideally, every change should be tested.
- The tests should be minimal. That is, they should be assembly files passed to 
llvm-mc rather than ll files passed to llc.

I'm not very familiar with RISC-V, so I'll leave further review to code owners.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D153229: [llvm] Move StringExtras.h include from Error.h to Error.cpp

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

Is SmallVector.h still required in Error.h?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153229

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

[PATCH] D153314: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: clang/lib/CodeGen/CGAtomic.cpp:1474
 
 Address AtomicInfo::emitCastToAtomicIntPointer(Address addr) const {
   llvm::IntegerType *ty =

as it no longer emits anything.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153314

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

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added inline comments.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1610
+}
+  } while (NonComments < 2 and ReadCount > 0);
+  return NextNextToken;

This is much more common.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D153229: [llvm] Move StringExtras.h include from Error.h to Error.cpp

[Sergei Barannikov via Phabricator via cfe-commits]

barannikov88 added a comment.

In D153229#134 , @IncludeGuardian 
wrote:

> @barannikov88 no it's not. I was going to commit separately to keep the 
> change small, but it turns out that if I move this to the source file there 
> are no additional changes needed. SmallVector.h has now been moved to 
> Error.cpp as well.

I guess it is transitively included from some other header, probably Twine.h.
LGTM with build fixed. I'll leave it for someone else to accept so that it does 
not disappear from review queue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153229

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