Page MenuHomePhabricator

Seperated DIBasicType DIFlags to DIBTFlags.
Needs ReviewPublic

Authored by Chirag on Feb 12 2020, 1:22 AM.

Details

Reviewers
aprantl
probinson
Summary

Currently DIBasicType uses two flags DIFlagBigEndian and DIFlagLittleEndian, this patch separates it to DIBTFlags.
DIBTFlags have two flags in this patch, DIBTFlagBigEndian and DIFlagLittleEndian.

This patch is in response for review https://reviews.llvm.org/D73861
After this patch, two more flags will be added in DIBTFlags.

The bitcode test is not present for DIBasicType with BigEndian/LittleEndian flags. this patch adds it.

Diff Detail

Event Timeline

Chirag created this revision.Feb 12 2020, 1:22 AM

Does this support the old metadata? We should have a test for the old metadata as well (e.g. flags: DIFlagBigEndian).

As we discussed on D73261, we might have to move a few flags from DIFlags into DISPFlags.

Please make a stack of this patch and the D73861.

Chirag updated this revision to Diff 244109.Feb 12 2020, 2:37 AM
  • Updated bitcode test to check type with proper variable.

Does this support the old metadata? We should have a test for the old metadata as well (e.g. flags: DIFlagBigEndian).

As we discussed on D73261, we might have to move a few flags from DIFlags into DISPFlags.

There is no bitcode testcase available for DIFlagBigEndian in llvm test suite, the current patch adds one bitcode test using DIBTFlagBigEndian/DIBTFlagLittleEndian.
The old metadata related bc files needs to be updated(if it uses DIFlagBigEndian) by DIBTFlag

Also for backward compatibility, the values of the flags can be kept the same.
<- HANDLE_DI_FLAG((1 << 27), BigEndian)
<- HANDLE_DI_FLAG((1 << 28), LittleEndian)

+ HANDLE_DIBT_FLAG((1 << 27), BigEndian) instead of 1
+ HANDLE_DIBT_FLAG((1 << 28), LittleEndian) instead of 2

This will keep the old bitcode format compatible, does this something you think might be usefull?

We can use one, currently unused, flag indicating the flags are moved. But, I would move more flags in that case (such as function-related flags that should go into the DISPFlags), not just the two.

We should handle old and new metadata within MetadataLoader, please take a look how that was done here: D59288.

Chirag updated this revision to Diff 244136.Feb 12 2020, 4:36 AM
  • Bitcode backward compatibility, added flag with distinct to mark BTFlags' presence.

D73261 would also benefit from moving more flags into the subprogram flags field. The refactoring really should move out as many flags as possible all in one go. I understand this is more than you anticipated but it will be the right thing to do for the project as a whole.

Thanks! This looks quite good already, I just have a few inline comments.

llvm/include/llvm/IR/DebugInfoMetadata.h
692

Basic

llvm/lib/AsmParser/LLParser.cpp
4174

Could the bulk of the implementation of this function be shared with the function that parses DIFlag and DISPFlag?

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1300

// BasicType-specific flags in DIFlag were moved to DIBTFlags.

1302

Thanks!

llvm/test/Bitcode/DIBasicType.ll
28

Assuming that the IR in this file is the input for DIBasicType.ll.bc, I would have expected this to use the *old* format to test that the old format can be upgraded to the new format. The assembler roundtrip test debug-info.ll is already testing that the new format works.

Anyhow, moving other flags should be a different revision (should not be part of this one), so with the comments addressed, this looks good to me. :)

llvm/include/llvm/IR/DebugInfoFlags.def
114

. at the end of the comment

llvm/test/Bitcode/DIBasicType.ll
28

+1

Chirag updated this revision to Diff 244342.Feb 13 2020, 12:08 AM
  • some typos cleanup.
  • updated bitcode test(both .bc and .ll files) to use old flags and checked against new btflags.
Chirag marked 6 inline comments as done.Feb 13 2020, 12:16 AM

D73261 would also benefit from moving more flags into the subprogram flags field. The refactoring really should move out as many flags as possible all in one go. I understand this is more than you anticipated but it will be the right thing to do for the project as a whole.

I can create a separate small patches for other flags DIVarFlags/Mod DISPFlag it will be easier to track if something gets broken. (i am not fully aware of all the flags' complete use cases)

D73261 would also benefit from moving more flags into the subprogram flags field. The refactoring really should move out as many flags as possible all in one go. I understand this is more than you anticipated but it will be the right thing to do for the project as a whole.

I can create a separate small patches for other flags DIVarFlags/Mod DISPFlag it will be easier to track if something gets broken. (i am not fully aware of all the flags' complete use cases)

That would be awesome! :)

Chirag added inline comments.Feb 13 2020, 2:05 AM
llvm/lib/AsmParser/LLParser.cpp
4174

Does using macro to generate flag LLParser seems like a good idea? it can be reused for other flags as well.

Chirag added inline comments.Feb 13 2020, 2:28 AM
llvm/lib/AsmParser/LLParser.cpp
4174

something like,

#define FLAG_FIELDS(MDNODE, FLAG_NAME) \
struct DIFLAG_NAMEField : public MDFieldImpl<MDNODE::DIFLAG_NAMEs> { \

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)

Chirag marked an inline comment as done.Feb 13 2020, 2:34 AM
Chirag added inline comments.
llvm/lib/AsmParser/LLParser.cpp
4174

i will create a reusable function for flag parsing.

Chirag marked an inline comment as not done.Feb 13 2020, 2:41 AM

D73261 would also benefit from moving more flags into the subprogram flags field. The refactoring really should move out as many flags as possible all in one go. I understand this is more than you anticipated but it will be the right thing to do for the project as a whole.

I can create a separate small patches for other flags DIVarFlags/Mod DISPFlag it will be easier to track if something gets broken. (i am not fully aware of all the flags' complete use cases)

While we usually always prefer the smallest possible patches for reviews, for moving attributes from one field to another in a bitcode-incompatible way, it would be better to move them in bulk, indicated by a single "version" bit in the bitcode encoding.

Chirag updated this revision to Diff 245809.Feb 21 2020, 3:05 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2020, 3:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

llvm/lib/AsmParser/LLParser.cpp
4174

Sharing the parsing code was not feasible?

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1515

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?

llvm/lib/IR/AsmWriter.cpp
1721

Again, can this function share code with the printDISPFlags function?

llvm/lib/IR/DebugInfoMetadata.cpp
366

Which warning is being appeased here?