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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM, but it'd be nice to have another approval as well.



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3626
+      if (SeenFirstFunctionBody)
+        return error("datalayout too late in module");
       std::string S;
----------------
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78403