This is an archive of the discontinued LLVM Phabricator instance.

[BitcodeReader] datalayout must be specified before it is queried.
ClosedPublic

Authored by efriedma on May 13 2020, 12:07 PM.

Details

Summary

This isn't really a new invariant; it effectively already existed due to existing DataLayout queries. But this makes it explicit.

This is technically not backward-compatible with the existing bitcode reader, but it's backward-compatible with the output of the bitcode writer, which is what matters in practice.

No testcase because I don't know a good way to write one.

Split off from D78403. (And modified a bit to handle the interaction between datalayout parsing and DataLayout::getProgramAddressSpace().)

Diff Detail

Event Timeline

efriedma created this revision.May 13 2020, 12:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 12:07 PM

We do have a bunch of tests that partially specify the datalayout to work around not properly setting it from the command line or target machine.

If you remove the datalayout string from test/CodeGen/AMDGPU/vector-alloca-bitcast.ll for example, does it work with this patch?

If you remove the datalayout string from test/CodeGen/AMDGPU/vector-alloca-bitcast.ll for example, does it work with this patch?

This patch doesn't have any effect except to reject certain unusual bitcode files not generated by LLVM.

If you're asking about D78403, that doesn't affect the general rules for what tools overwrite the datalayout based on the TargetMachine. Currently, llc always does this, and opt never does this. So the error messages for that sort of thing should remain the same.

jdoerfert added inline comments.May 13 2020, 5:36 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

Should we emit an error here? I mean, can it happen we now silently ignore the DL or triple and the user is baffled what is happening?

efriedma marked an inline comment as done.May 13 2020, 6:11 PM
efriedma added inline comments.
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

We print an error if we see MODULE_CODE_DATALAYOUT or MODULE_CODE_TRIPLE after ResolveDataLayout runs.

jdoerfert added inline comments.May 13 2020, 6:54 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

I guess we could test that?

mehdi_amini added inline comments.May 13 2020, 7:38 PM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

That would require crafting manually some invalid bitcode wouldn't it?

mehdi_amini accepted this revision.May 13 2020, 7:39 PM
mehdi_amini added a reviewer: pcc.

LGTM (but worth seeing if @pcc has an opinion on this)

This revision is now accepted and ready to land.May 13 2020, 7:39 PM
jdoerfert added inline comments.May 14 2020, 10:34 AM
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

Yes, or better, something we now call invalid. @efriedma please correct me but there could be an input we accepted before but not now?

efriedma marked an inline comment as done.May 14 2020, 11:28 AM
efriedma added inline comments.
llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

Yes. But no tools exist to generate such a file.

I don't really want to sink a day into constructing a framework for BitcodeReader unittests, but if you think it's necessary, I guess I can.

jdoerfert accepted this revision.May 14 2020, 11:33 AM

LGTM

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
3445

No, it was more of an idea, not a request.

This revision was automatically updated to reflect the committed changes.