This is an archive of the discontinued LLVM Phabricator instance.

Restore the capability to manipulate datalayout from the C API
ClosedPublic

Authored by deadalnix on Feb 15 2016, 1:03 AM.

Details

Summary

This consist in variosu addition to the C API:

LLVMTargetDataRef LLVMGetModuleDataLayout(LLVMModuleRef M);
void LLVMSetModuleDataLayout(LLVMModuleRef M, LLVMTargetDataRef DL);
LLVMTargetDataRef LLVMCreateTargetMachineData(LLVMTargetMachineRef T);

This also removes LLVMAddTargetData, which does nothing and is historical cruft.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 47957.Feb 15 2016, 1:03 AM
deadalnix retitled this revision from to Restore the capability to manipulate datalayout from the C API.
deadalnix updated this object.
mehdi_amini added inline comments.Feb 15 2016, 1:54 PM
include/llvm-c/Target.h
189–190

Separate change?

191

Not clear why it isn't at the same place as LLVMGetDataLayout

195

comment seems wrong

203

split code motion in a separate patch (you may commit it and rebase this one).

include/llvm-c/TargetMachine.h
91

remove

include/llvm/IR/DataLayout.h
471 ↗(On Diff #47957)

do not mix formatting change with other changes please

lib/IR/Core.cpp
166 ↗(On Diff #47957)

Are you planning to deprecate this? Why a new duplicated API?

173 ↗(On Diff #47957)

You can commit this change separately now.

lib/Target/TargetMachineC.cpp
177

I don't really like how we're not consistent on the naming. "Data" instead of "DataLayout" has always bothered me.
(Just mentioning, not your fault).

mehdi_amini edited edge metadata.Feb 15 2016, 1:55 PM

Oh, and since it is your domain: what about the tests?
Can you add a triple and a random data layout (not the default one) to the echo test?

deadalnix added inline comments.Feb 15 2016, 3:38 PM
include/llvm-c/Target.h
191

LLVMGetDataLayout really should be named LLVMGetDataLayoutStr as it maps getDataLayourStr, which is the string representation of the layout.

lib/IR/Core.cpp
166 ↗(On Diff #47957)

Yes, the name is not only plain wrong but is also the name of anther method that does something else, which is confusing.

173 ↗(On Diff #47957)

Got you. Will do.

lib/Target/TargetMachineC.cpp
177

Agreed. Problem is, the good names are taken here already, so I had to come up with something.

mehdi_amini added inline comments.Feb 15 2016, 3:41 PM
include/llvm-c/Target.h
191

This is a getter on the module right? I'm asking about the location in the header: why here and not in include/llvm-c/Core.h where the other getter for Module are?

lib/IR/Core.cpp
166 ↗(On Diff #47957)

So mark it deprecated with a mention to use the other one. Also this sounds like a separate change to the C API (i.e. I'd say a separate review).

lib/Target/TargetMachineC.cpp
177

LLVMCreateTargetDataLayout ? LLVMCreateDataLayoutForTarget ?

deadalnix added inline comments.Feb 15 2016, 3:47 PM
include/llvm-c/Target.h
191

Yes, but LLVMTargetDataRef is not available from Core.h and making it so would be a bigger change. Also, all LLVMTargetDataRef related method are here, so I don't think this is that much of an issue.

deadalnix updated this revision to Diff 48030.Feb 15 2016, 4:05 PM
deadalnix edited edge metadata.

Update as per comments

mehdi_amini added inline comments.Feb 15 2016, 4:11 PM
include/llvm-c/Target.h
191

LLVMTargetDataRef is not available from Core.h

That's a good reason :)

195

comment is still wrong?

lib/Target/TargetMachineC.cpp
177

What about one of these name? (you haven't answered so I'm not even sure you saw it)

I also asked earlier about adding a data layout and a triple in the "echo" test?

deadalnix updated this revision to Diff 48032.Feb 15 2016, 4:16 PM

Fix comment
Update name

deadalnix updated this revision to Diff 48040.Feb 15 2016, 4:41 PM

Add check in the echo C API test

mehdi_amini accepted this revision.Feb 15 2016, 4:43 PM
mehdi_amini edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Feb 15 2016, 4:43 PM
This revision was automatically updated to reflect the committed changes.