This is an archive of the discontinued LLVM Phabricator instance.

Bitcode: Move version and global value module code parsers to separate functions. NFCI.
ClosedPublic

Authored by pcc on Apr 7 2017, 2:24 PM.

Event Timeline

pcc created this revision.Apr 7 2017, 2:24 PM
tejohnson added inline comments.Apr 10 2017, 12:09 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

Why move this into BitcodeReaderBase, rather than BitcodeReader?

pcc added inline comments.Apr 10 2017, 2:18 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

In D31838 the code that reads names is shared between the module reader and the summary reader. That code needs to know the module version, so I needed to move the module version parsing code into the base class, including the fields that it sets.

tejohnson added inline comments.Apr 11 2017, 7:24 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

Ok, but if this field is only ever going to be relevant to the BitcodeReader derived class, why not have a virtual method that takes the parsed version and can be used to set up whatever fields are relevant for each derived class? I would imagine this won't be the last time we end up needing to guard behavior specific to only one derived reader based on the version.

pcc added inline comments.Apr 11 2017, 10:13 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

Such a method would be called from {,ModuleSummaryIndex}BitcodeReader:;parseModule which is already specific to the derived class. If we do what you suggest we might as well inline the parser code into the parseModule methods, i.e. what we are already doing. Would you prefer that? I suppose I have no strong objections to it, but figured it may be better to keep track of the version evolution in one place.

tejohnson added inline comments.Apr 11 2017, 1:27 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

Actually I was thinking of a pure virtual method that would be called from BitcodeReaderBase::parseVersionRecord and passed the parsed ModuleVersion number. Then each derived reader would define this method as well as the flags specific to that reader. That way this flag could be moved into BitcodeReader, whereas something like the strtab flag being added in your other patch would be defined in BitcodeReaderBase (and set in parseVersionRecord) since it applies to all.

pcc added inline comments.Apr 11 2017, 2:02 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

That doesn't really seem like an improvement on the status quo, though (at least IMHO). I'd rather just set everything in the base class, as it's simpler than trying to delegate to the derived classes.

That said I'd be fine with either approach, so let me know if you still want to change it.

tejohnson added inline comments.Apr 11 2017, 9:36 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
388

I have a preference for keeping the flags in the derived class if they are specific to it. It seems cleaner to me and avoids needing to document which flags are applicable to which class(es).

pcc updated this revision to Diff 95021.Apr 12 2017, 12:58 PM
pcc marked an inline comment as done.
  • Move the flag back
This revision is now accepted and ready to land.Apr 12 2017, 1:08 PM
This revision was automatically updated to reflect the committed changes.