[PATCH] D78403: Infer alignment of loads with unspecified alignment in IR/bitcode parsing.

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked an inline comment as done.
efriedma added inline comments.


================
Comment at: llvm/tools/opt/opt.cpp:123
 static cl::opt<bool>
+NoUpgradeDebugInfo("disable-upgrade-debug-info",
+                   cl::desc("Generate invalid output"), cl::ReallyHidden);
----------------
aprantl wrote:
> dexonsmith wrote:
> > efriedma wrote:
> > > mehdi_amini wrote:
> > > > How is it related to the data layout thing?
> > > Upgrading debug info encounters a similar sort of issue with the datalayout as computing the alignment: if we try to upgrade debug info with the wrong datalayout, we get the wrong result.  llc was using the UpgradeDebugInfo boolean to delay upgrading debug info until after it computed the correct datalayout.  With this patch, llc doesn't need that special case anymore.
> > > 
> > > Given that, there isn't really any legitimate use for disabling UpgradeDebugInfo anymore.  So I decided it makes sense to split apart the API, so the normal entry points for IR parsing don't expose this weird edge case of intentionally creating invalid bitcode files.  I added the entry point parseAssemblyFileWithIndexNoUpgradeDebugInfo, and I split the debug-upgrading part of "-disable-verify" into a separate option.
> > > 
> > > Maybe I should do something more minimal in this patch?
> > It would be great for @aprantl to review at least this part of the patch.
> > 
> > @efriedma, I'm curious if this refactoring would be easy/reasonable to split out and commit first?
> > Given that, there isn't really any legitimate use for disabling UpgradeDebugInfo anymore.
> 
> IIRC I originally added this option for the sole reason of testing the upgrade behavior, as shown by the doxygen comment:
> 
> ```
> /// \param UpgradeDebugInfo Run UpgradeDebugInfo, which runs the Verifier.
> ///                         This option should only be set to false by llvm-as
> ///                         for use inside the LLVM testuite!
> ```
> 
> The intended behavior is that if you run llvm-as --disable-verify (something that should not be done outside of the testsuite) this flag would be set to false.
> 
> See https://reviews.llvm.org/D38184 for the original context.
I guess I could split out the change to add this flag into a separate patch.  I could split out the actual change to set the alignment of LoadInst into a separate patch.  I guess I could also split out the part of the IR parsing change that requires the datalayout at the beginning of the file.  I don't think any of those changes help make the patch more comprehensible, but they would make it smaller.

I can't really split the IR parsing API rewrite in any reasonable way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78403