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

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked 2 inline comments as done.
efriedma added inline comments.


================
Comment at: llvm/include/llvm/AsmParser/Parser.h:31
 
+typedef llvm::function_ref<void (Module*)> DataLayoutCallbackTy;
+
----------------
mehdi_amini wrote:
> The type for the callback is strange to me, this allows basically any modification of the Module during the callback, can't we just pass an `Optional<DataLayout> override_datalayout` in the parsing functions?
llc needs access to the target triple from the IR file to decide which datalayout to use.  Hence the callback API: we need to decide the datalayout after we parse the triple, but before we parse any function definitions.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78403