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



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

Reply via email to