[PATCH] Refactor bitcode reader to simplify control logic.

[Duncan P. N. Exon Smith]

> On 2015 Jun 17, at 13:21, Karl Schimpf <kschimpf at google.com> wrote:
> 
> Hi dschuff, jvoung, rafael, filcab,
> 
> Modifies the bitcode reader to use the same logic for both memory
> buffers and data streams (other than initialization).  The incremental
> parser is now factored into startParse, continueParse, and
> finishParse. All parses begin with startParse. It then makes zero or
> more calls to continueParse (via incremental parsing). Finally, when
> materializing, finishParse to finish parsing.
> 
> Note that this CL replaces http://reviews.llvm.org/D8786.
> 
> http://reviews.llvm.org/D10518
> 
> Files:
>  include/llvm/Bitcode/BitstreamReader.h
>  lib/Bitcode/Reader/BitcodeReader.cpp
>  test/Bitcode/Inputs/invalid-abbrev.bc
>  test/Bitcode/Inputs/invalid-data-after-module.bc
>  test/Bitcode/Inputs/invalid-fwdref-type-mismatch-2.bc
>  test/Bitcode/Inputs/invalid-load-ptr-type.bc
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> <D10518.27866.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

This is a fairly monolithic patch.  Have you thought about how you could
do this more incrementally?

In particular, if you're going to make semantic changes like the ones
I've highlighted below, they should be done separately from a big reorg
like this.

> Index: lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- lib/Bitcode/Reader/BitcodeReader.cpp
> +++ lib/Bitcode/Reader/BitcodeReader.cpp
> @@ -4462,27 +4613,23 @@
>    assert(M == TheModule &&
>           "Can only Materialize the Module this BitcodeReader is attached to.");
>  
> -  if (std::error_code EC = materializeMetadata())

Why have you changed the order of materializing metadata?

> +  // Make sure the rest of the bits in the module (excluding materializable)
> +  // have been read.
> +  if (std::error_code EC = finishParse())
>      return EC;
>  
> -  // Promise to materialize all forward references.
> -  WillMaterializeAllForwardRefs = true;

This promise limited the potential performance impact of r214559.  Why
have you removed it?

Also, I think this will change the use-list order in the presence of
`blockaddress` instructions.  I guess testing for that might be
insufficient, or maybe I'm missing something here.  Now that you've
removed this promise, won't the order of parsing functions change if
there's a forward reference to a blockaddress?

> +  if (std::error_code EC = materializeMetadata())
> +    return EC;
>  
>    // Iterate over the module, deserializing any functions that are still on
>    // disk.
>    for (Module::iterator F = TheModule->begin(), E = TheModule->end();
>         F != E; ++F) {
>      if (std::error_code EC = materialize(F))
>        return EC;
>    }