[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/lib/Bitcode/Reader/BitcodeReader.cpp:3626
+      if (SeenFirstFunctionBody)
+        return error("datalayout too late in module");
       std::string S;
----------------
mehdi_amini wrote:
> efriedma wrote:
> > mehdi_amini wrote:
> > > Are you making invalid bitcode that was valid before here?
> > > I'd rather avoid doing this, can we check `if (SeenFirstFunctionBody && DataLayoutCallback)`?
> > > (which also implies to not provide a callback from any tool when not needed (and make sure the default argument for the callback is everywhere an empty function_ref)
> > Yes, this is rejecting bitcode that LLVM currently accepts.  No version of LLVM has ever emitted bitcode that violates this constraint, though; LLVM always emits the datalayout early.  In theory, I guess if someone had a custom bitcode emitter that emitted bitcode in some really weird order, it could be an issue.
> > 
> > I'm not sure what we would do if we want to "handle" this; if we see a datalayout after we've already read IR instructions, we would have to go back and fix/reparse already parsed IR.  I guess we could, but I'm not really enthusiastic about writing bitcode testcases by hand to test that codepath.
> > if we see a datalayout after we've already read IR instructions, we would have to go back and fix/reparse already parsed IR
> 
> This would already be an issue with the current parser though?
> If you don't mind committing these two lines right now since it seems like a defensive measure in the reader that is valid to do independently of this patch (assuming I understand correctly here?)
The current parser doesn't really query the datalayout.  (Well, except in the cases where it does, for the program/stack address spaces.  But that doesn't affect mainstream targets.)

Yes, sure, I can commit this separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78403