This is an archive of the discontinued LLVM Phabricator instance.

Remove access to the DataLayout in the TargetMachine
ClosedPublic

Authored by mehdi_amini on Jul 10 2015, 11:09 AM.

Details

Summary

Replace getDataLayout() with a createDataLayout() method to make
explicit that it is intended to create a DataLayout only and not
accessing it for other purpose.

This change is the last of a series of commits dedicated to have a
single DataLayout during compilation by using always the one owned
by the module.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Remove access to the DataLayout in the TargetMachine.
mehdi_amini updated this object.
mehdi_amini added a reviewer: echristo.
mehdi_amini added inline comments.Jul 10 2015, 11:10 AM
lib/Target/TargetMachineC.cpp
174

Please review this in particular

Update based on D11110 (ExecutionEngine owning a DataLayout now)

Cleanup, previous update was incomplete.

echristo edited edge metadata.Jul 10 2015, 6:15 PM

I think parts of this are going to conflict with another patch. I'll wait for an update before commenting further.

-eric

mehdi_amini edited edge metadata.

Rebase (minor conflict)

rafael added inline comments.Jul 16 2015, 8:40 AM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
143

For another patch, but could the AsmPrinter constructor take the pointer size?

lib/ExecutionEngine/MCJIT/MCJIT.cpp
71 ↗(On Diff #29868)

Can't it use the DL from the module?

lib/ExecutionEngine/Orc/OrcMCJITReplacement.h
143

same here.

160

whitespace change.

lib/LTO/LTOCodeGenerator.cpp
524

I think this is from the days when we supported the module not having a DL. We should probably try to remove it in another patch.

lib/LTO/LTOModule.cpp
235

same here.

Thanks for the review Raphael.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
143

Yes it is an option, I'll look into it.

lib/ExecutionEngine/MCJIT/MCJIT.cpp
71 ↗(On Diff #29868)

The problem is that the JIT supports adding a module but also removing a module from its list. Keeping a reference to the DL of the module used for creation seems dangerous on this aspect.

lib/LTO/LTOCodeGenerator.cpp
524

I'm not very familiar with the JIT use case, but how is the client creating the module? And how can he makes sure that the DL on the module that he will give to the JIT is the one the TargetMachine expects?
Is he supposed to keep a reference to the TargetMachine when creating the JIT?

rafael wrote:

I think this is from the days when we supported the module not having a DL. We should probably try to remove it in another patch.

I'm not very familiar with the JIT use case, but how is the client creating the module? And how can he makes sure that the DL on the module that he will give to the JIT is the one the TargetMachine expects?
Is he supposed to keep a reference to the TargetMachine when creating the JIT?

I am not too familiar with the JIT, but that is probably a reasonable
requirement for LTO.

Cheers,
Rafael

lhames added inline comments.Jul 16 2015, 1:19 PM
lib/LTO/LTOCodeGenerator.cpp
524

This is LTO code, and the LTO and JIT cases may be different.

Historically the JIT hasn't required a data layout. That seems like a reasonable restriction for us to add, but we'd have to give clients a heads-up first. For now, the JIT code should still set the data layout if there isn't one present.

echristo added inline comments.Jul 16 2015, 1:26 PM
lib/LTO/LTOCodeGenerator.cpp
524

Well, you have, you're just relied on the TargetMachine for this. Also, I imagine there's a DataLayout in the module since it's required now anyhow right? :)

lhames added inline comments.Jul 16 2015, 1:36 PM
lib/LTO/LTOCodeGenerator.cpp
524

Sorry: To clarify, what I meant was that we haven't historically required JIT clients to supply Modules with a DataLayout attached. In fact, until fairly recently MCJIT would happily consume & codegen IR without a DataLayout.

When I wrote Orc I added the data layout requirement to ensure that symbol naming was consistent between JIT'd and static code. To avoid changing the contract on clients, MCJIT and the in-tree Orc stacks (OrcMCJITReplacement, OrcLazy) take a TargetMachine and use it to attach a DataLayout if there isn't already one present.

I'd be open to changing the contract on MCJIT and Orc to require a DataLayout on the modules if that would be more consistent with LTO.

echristo added inline comments.Jul 16 2015, 1:43 PM
lib/LTO/LTOCodeGenerator.cpp
524

That makes sense. After Mehdi's earlier patches DataLayout is required on the Module, but we're probably not enforcing it. I'm working on how to help fix consumers of the TargetMachine DataLayout construction routines to construct a DataLayout ahead of time.

Fix the C API for backward compatibility

The C API back compat looks plausible to me - but it's not an area I dabble in, so I've no idea how legitimate it is as an idea. Totally seems like a fair way to do back-compat.

lib/Target/TargetMachineC.cpp
41

This could be a unique_ptr<TargetMachine>, perhaps?

Update the C API using a std::unique_ptr as suggested by D. Blaikie

echristo accepted this revision.Jul 23 2015, 6:44 PM
echristo edited edge metadata.

Looks like a good intermediate way to go.

-eric

This revision is now accepted and ready to land.Jul 23 2015, 6:44 PM