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

Mon Mar 16 09:14:54 PDT 2020

thakis added inline comments.

================
Comment at: clang/utils/TableGen/SveEmitter.cpp:32
 #include "llvm/TableGen/Error.h"
+#include "clang/Basic/AArch64SVETypeFlags.h"
 #include <string>
----------------
sdesmalen wrote:
> 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?
That seems like a strange place for this header.

Maybe you can rework things so that you don't have to share a header between clang's tablegen and clang's Basic? No other tablegen output so far has needed that. (see e.g. the `  /// These must be kept in sync with the flags in utils/TableGen/NeonEmitter.h.` line in TargetBuiltins.h).

If that isn't possible at all, I suppose you could put the .h file in clang/utils/TableGen and also make clang-tblgen write the .h file and use the written .h file in Basic.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75470