[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 ) {
+

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 
> > {  \
> >   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 ) { 
> >   \
> > 
> >   \
> >   auto parseFlag = [&](MDNODE::DI##FLAG_NAME##s ) { 
> >   \
> > if (Lex.getKind() == lltok::APSInt && !Lex.getAPSIntVal().isSigned()) { 
> >   \
> >   uint32_t TempVal = static_cast(Val);
> >   \
> >   bool Res = ParseUInt32(TempVal);  
> >   \
> >   Val = static_cast(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)   

[PATCH] D74470: Seperated DIBasicType DIFlags to DIBTFlags.

[Chirag Patel via Phabricator via cfe-commits]

Chirag updated this revision to Diff 245809.
Chirag added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Moved five of the DIFlags to DISPFlags. (updated few clang testcases)
DIFlagExplicit -> DISPFlagExplicit
DIFlagPrototyped -> DISPFlagPrototyped
DIFlagNoReturn -> DISPFlagNoReturn
DIFlagThunk -> DISPFlagThunk
DIFlagAllCallsDescribed -> DISPFlagAllCallsDescribed

Note: currently llvm ir parser still needs the DIFlags, once the llvm ir format 
gets updated(with all the relevant testcases) the entries from 
DebugInfoFlags.def will be removed and moveDItoSP should only be used for 
bitcode read and C-API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74470

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp
  clang/test/CodeGenCXX/debug-info-access.cpp
  clang/test/CodeGenCXX/debug-info-cxx1y.cpp
  clang/test/CodeGenCXX/debug-info-decl-nested.cpp
  clang/test/CodeGenCXX/debug-info-deleted.cpp
  clang/test/CodeGenCXX/debug-info-ms-abi.cpp
  clang/test/CodeGenCXX/debug-info-noreturn.cpp
  clang/test/CodeGenCXX/debug-info-qualifiers.cpp
  clang/test/CodeGenCXX/debug-info-thunk.cpp
  clang/test/CodeGenObjC/debug-info-category.m
  llvm/include/llvm/IR/DIBuilder.h
  llvm/include/llvm/IR/DebugInfoFlags.def
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/MetadataLoader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/lib/IR/DebugInfoMetadata.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/test/Assembler/debug-info.ll
  llvm/test/Assembler/disubprogram.ll
  llvm/test/Bitcode/DIBasicType.ll
  llvm/test/Bitcode/DIBasicType.ll.bc
  llvm/test/Bitcode/DISubprogram-v5.ll
  llvm/test/DebugInfo/COFF/thunk.ll
  llvm/unittests/IR/MetadataTest.cpp

Index: llvm/unittests/IR/MetadataTest.cpp
===
--- llvm/unittests/IR/MetadataTest.cpp
+++ llvm/unittests/IR/MetadataTest.cpp
@@ -1202,9 +1202,8 @@
 typedef MetadataTest DIBasicTypeTest;
 
 TEST_F(DIBasicTypeTest, get) {
-  auto *N =
-  DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33, 26, 7,
-DINode::FlagZero);
+  auto *N = DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
+ 26, 7, DIBasicType::BTFlagZero);
   EXPECT_EQ(dwarf::DW_TAG_base_type, N->getTag());
   EXPECT_EQ("special", N->getName());
   EXPECT_EQ(33u, N->getSizeInBits());
@@ -1213,31 +1212,31 @@
   EXPECT_EQ(0u, N->getLine());
   EXPECT_EQ(DINode::FlagZero, N->getFlags());
   EXPECT_EQ(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
-26, 7, DINode::FlagZero));
+26, 7, DIBasicType::BTFlagZero));
 
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_unspecified_type,
-"special", 33, 26, 7, DINode::FlagZero));
-  EXPECT_NE(N,
-DIBasicType::get(Context, dwarf::DW_TAG_base_type, "s", 33, 26, 7,
-  DINode::FlagZero));
+"special", 33, 26, 7, DIBasicType::BTFlagZero));
+  EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "s", 33, 26,
+7, DIBasicType::BTFlagZero));
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 32,
-26, 7, DINode::FlagZero));
+26, 7, DIBasicType::BTFlagZero));
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
-25, 7, DINode::FlagZero));
+25, 7, DIBasicType::BTFlagZero));
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
-26, 6, DINode::FlagZero));
+26, 6, DIBasicType::BTFlagZero));
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
-26, 7, DINode::FlagBigEndian));
+26, 7, DIBasicType::BTFlagBigEndian));
   EXPECT_NE(N, DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special", 33,
-26, 7, DINode::FlagLittleEndian));
+26, 7, DIBasicType::BTFlagLittleEndian));
 
   TempDIBasicType Temp = N->clone();
   EXPECT_EQ(N, MDNode::replaceWithUniqued(std::move(Temp)));
 }
 
 TEST_F(DIBasicTypeTest, getWithLargeValues) {
-  auto *N = DIBasicType::get(Context, dwarf::DW_TAG_base_type, "special",
- UINT64_MAX, UINT32_MAX - 1, 7, DINode::FlagZero);
+  auto *N =
+