[PATCH] D74470: Seperated DIBasicType DIFlags to DIBTFlags.

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Thank you this looks very good! Just a few questions inline.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:4171
+template <>
+bool LLParser::ParseMDField(LocTy Loc, StringRef Name, DIBTFlagField &Result) {
+
----------------
Chirag wrote:
> Chirag wrote:
> > Chirag wrote:
> > > aprantl wrote:
> > > > Could the bulk of the implementation of this function be shared with the function that parses DIFlag and DISPFlag?
> > > Does using macro to generate flag LLParser seems like a good idea? it can be reused for other flags as well.
> > something like,
> > 
> > #define FLAG_FIELDS(MDNODE, FLAG_NAME)                                        \
> > struct DI##FLAG_NAME##Field : public MDFieldImpl<MDNODE::DI##FLAG_NAME##s> {  \
> >   DI##FLAG_NAME##Field() : MDFieldImpl(MDNODE::FLAG_NAME##Zero) {}            \
> > };
> > 
> > FLAG_FIELDS(DINode, Flag)
> > FLAG_FIELDS(DIBasicType, BTFlag)
> > FLAG_FIELDS(DISubprogram, SPFlag)
> > 
> > 
> > #define FLAG_FIELDS_PARSER(MDNODE, FLAG_NAME)                                 \
> > template <>                                                                   \
> > bool LLParser::ParseMDField(LocTy Loc, StringRef Name,                        \
> >                             DI##FLAG_NAME##Field &Result) {                   \
> >                                                                               \
> >   auto parseFlag = [&](MDNODE::DI##FLAG_NAME##s &Val) {                       \
> >     if (Lex.getKind() == lltok::APSInt && !Lex.getAPSIntVal().isSigned()) {   \
> >       uint32_t TempVal = static_cast<uint32_t>(Val);                          \
> >       bool Res = ParseUInt32(TempVal);                                        \
> >       Val = static_cast<MDNODE::DI##FLAG_NAME##s>(TempVal);                   \
> >       return Res;                                                             \
> >     }                                                                         \
> >                                                                               \
> >     if (Lex.getKind() != lltok::DI##FLAG_NAME)                                \
> >       return TokError("expected debug info flag");                            \
> >                                                                               \
> >     Val = MDNODE::getFlag(Lex.getStrVal());                                   \
> >     if (!Val)                                                                 \
> >       return TokError(Twine("invalid basicType debug info flag '") +          \
> >                       Lex.getStrVal() + "'");                                 \
> >     Lex.Lex();                                                                \
> >     return false;                                                             \
> >   };                                                                          \
> >                                                                               \
> >   MDNODE::DI##FLAG_NAME##s Combined = MDNODE::FLAG_NAME##Zero;                \
> >   do {                                                                        \
> >     MDNODE::DI##FLAG_NAME##s Val;                                             \
> >     if (parseFlag(Val))                                                       \
> >       return true;                                                            \
> >     Combined |= Val;                                                          \
> >   } while (EatIfPresent(lltok::bar));                                         \
> >                                                                               \
> >   Result.assign(Combined);                                                    \
> >   return false;                                                               \
> > }
> > 
> > FLAG_FIELDS_PARSER(DINode, Flag)
> > FLAG_FIELDS_PARSER(DIBasicType, BTFlag)
> > FLAG_FIELDS_PARSER(DISubprogram, SPFlag)
> > 
> > 
> i will create a reusable function for flag parsing. 
Sharing the parsing code was not feasible?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1515
+    // Some flags were moved from DIFlags to SPFlags
+    DISubprogram::moveDItoSPFlags(Flags, SPFlags);
+
----------------
I believe this should only be called conditionally? Or specifically: What will happen when we reuse flag bits that are in the range of the old now freed-up DIFlags?


================
Comment at: llvm/lib/IR/AsmWriter.cpp:1721
 
+void MDFieldPrinter::printDIBTFlags(StringRef Name,
+                                    DIBasicType::DIBTFlags Flags) {
----------------
Again, can this function share code with the printDISPFlags function?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:366
+  switch (Flag) {
+    // Appease a warning.
+#define HANDLE_DIBT_FLAG(ID, NAME)                                             \
----------------
Which warning is being appeased here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74470