[PATCH] D75470: [SVE] Auto-generate builtins and header for svld1.

[Sander de Smalen via Phabricator via cfe-commits]

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


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:32
 #include "llvm/TableGen/Error.h"
+#include "clang/Basic/AArch64SVETypeFlags.h"
 #include <string>
----------------
thakis wrote:
> Including stuff from `clang/Basic` in clang/utils/TableGen is conceptually a layering violation: clang-tblgen is used to generate headers included in clang/Basic. In this case it happens to work, but it's because you're lucky, and it could break for subtle reasons if the TypeFlags header starts including some other header in Basic that happens to include something generated.
> 
> Please restructure this so that the TableGen code doesn't need an include from Basic.
Thanks for pointing out! The only directory that seems to have common includes between Clang TableGen/CodeGen is the llvm/Support directory, any objection to me moving the header there?


================
Comment at: clang/utils/TableGen/TableGen.cpp:196
+        clEnumValN(GenArmSveCodeGenMap, "gen-arm-sve-codegenmap",
+                   "Generate arm_sve_codegenmap.inc for clang"),
         clEnumValN(GenArmMveHeader, "gen-arm-mve-header",
----------------
thakis wrote:
> Any reason these aren't called `-gen-arm-sve-builtin-def` and `-gen-arm-sve-builtin-codegen` for consistency with CDE and MVE?
Not really, I can change that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75470