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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/include/llvm/AsmParser/Parser.h:65
 ///                         This option should only be set to false by llvm-as
 ///                         for use inside the LLVM testuite!
 /// \param DataLayoutString Override datalayout in the llvm assembly.
----------------
There is a stale doxygen comment here now.


================
Comment at: llvm/tools/opt/opt.cpp:123
 static cl::opt<bool>
+NoUpgradeDebugInfo("disable-upgrade-debug-info",
+                   cl::desc("Generate invalid output"), cl::ReallyHidden);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78403