[PATCH] D156184: [OpenMP] Add the `ompx_attribute` clause for target directives

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:29
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "llvm/ADT/ArrayRef.h"
----------------
Bad to have Sema dependancy in AST.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:9190
+  /// The parsed attributes (clause arguments)
+  SmallVector<const Attr *> Attrs;
+
----------------
Use tail allocation


================
Comment at: clang/include/clang/AST/OpenMPClause.h:9214
+  /// Returned the attributes parsed from this clause.
+  ArrayRef<const Attr *> getAttrs() const { return Attrs; }
+
----------------
Drop const modifier


================
Comment at: clang/include/clang/AST/OpenMPClause.h:9217
+  /// Replace the attributes with \p NewAttrs.
+  void setAttrs(ArrayRef<Attr *> NewAttrs) {
+    Attrs.clear();
----------------
Make this private member function


================
Comment at: clang/include/clang/Sema/Sema.h:12355
+  /// Called on a well-formed 'ompx_attribute' clause.
+  OMPClause *ActOnOpenMPXAttributeClause(ArrayRef<const Attr *> Attrs,
+                                         SourceLocation StartLoc,
----------------
Drop const


================
Comment at: clang/lib/AST/ExprConstant.cpp:4942
 /// statement should be stored.
-struct StmtResult {
+struct StmtResultTy {
   /// The APValue that should be filled in with the returned value.
----------------
Not related change


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1561
+  /// Emit the IR encoding to attach the CUDA launch bounds attribute to \p F.
+  void HandleCUDALaunchBoundsAttr(llvm::Function *F,
+                                  const CUDALaunchBoundsAttr *A);
----------------
handleCUDA....


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1567
+  /// ReqdWGS.
+  void HandleAMDGPUFlatWorkGroupSizeAttr(
+      llvm::Function *F, const AMDGPUFlatWorkGroupSizeAttr *A,
----------------
handle...


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1572
+  /// Emit the IR encoding to attach the AMD GPU waves-per-eu attribute to \p F.
+  void HandleAMDGPUWavesPerEUAttr(llvm::Function *F,
+                                  const AMDGPUWavesPerEUAttr *A);
----------------
handle...


================
Comment at: clang/lib/CodeGen/Targets/AMDGPU.cpp:564
+void CodeGenModule::HandleAMDGPUFlatWorkGroupSizeAttr(
+    llvm::Function *F, const AMDGPUFlatWorkGroupSizeAttr *FlatWGS,
+    const ReqdWorkGroupSizeAttr *ReqdWGS) {
----------------
Pass by reference what's possible?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:49
 #include "llvm/Support/raw_ostream.h"
+#include <clang/AST/Attrs.inc>
 #include <optional>
----------------
Sort includes


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7941
+
+  return ::new (Context) AMDGPUWavesPerEUAttr(Context, CI, MinExpr, MaxExpr);
+}
----------------
Use Create member function?


================
Comment at: clang/lib/Sema/TreeTransform.h:2384
+  /// Subclasses may override this routine to provide different behavior.
+  OMPClause *RebuildOMPXAttributeClause(ArrayRef<const Attr *> Attrs,
+                                        SourceLocation StartLoc,
----------------
Drop const


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

https://reviews.llvm.org/D156184