This is an archive of the discontinued LLVM Phabricator instance.

Emit the ARM build attributes ABI_PCS_wchar_t and ABI_enum_size.
ClosedPublic

Authored by olista01 on Jun 19 2014, 6:46 AM.

Details

Reviewers
compnerd
Summary

Emit the ARM build attributes ABI_PCS_wchar_t and ABI_enum_size based on module flags metadata.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 10634.Jun 19 2014, 6:46 AM
olista01 retitled this revision from to Emit the ARM build attributes ABI_PCS_wchar_t and ABI_enum_size..
olista01 updated this object.
olista01 edited the test plan for this revision. (Show Details)
olista01 added a subscriber: Unknown Object (MLST).
rengolin added inline comments.
include/llvm/Support/ARMBuildAttributes.h
163

you're never using, nor testing this...

175

you're never using, nor testing this...

180

you're never using, nor testing this...

test/CodeGen/ARM/metadata-default.ll
14

shouldn't this value be the actual number of bytes used? so than you can forbid usage from the front-end later.

15

I think this one could be a bit more expressive, such as using a string, so that we can disable or use the fourth style, which is currently inaccessible.

compnerd accepted this revision.Jun 19 2014, 9:38 AM
compnerd added a reviewer: compnerd.
compnerd added a subscriber: compnerd.

Some minor nits, but LGTM. Thanks for adding support for this EABI attribute.

include/llvm/Support/ARMBuildAttributes.h
162

Can you add the uleb128 encoding and type similar to the other enums please?

174

Same here.

lib/Target/ARM/ARMAsmPrinter.cpp
736

You should be able to collapse the cast and if into a single statement.

I think you can use auto here without loss of clarity:

if (auto WCharType = cast_or_null<ConstantInt>(SM->getModuleFlag("short_wchar"))
746

Similarly here.

test/CodeGen/ARM/metadata-default.ll
5

The Module ID doesnt add much to the test. You can trim the test to just the RUN line, the define, and metadata.

This revision is now accepted and ready to land.Jun 19 2014, 9:38 AM

you're never using, nor testing this... x3

I was following the example of the other build attributes (e.g. the position independent code ones), including all values specified in the ABI addenda, even if we do not use them.

olista01 updated this revision to Diff 10686.Jun 20 2014, 2:14 AM
olista01 edited edge metadata.

Changes corresponding to matching change in D4212 (store sizes of wchar_t and enums in metadata, not just boolean flags)

Hi Oliver,

I'm happy with this patch, but we need to make sure we don't create those attributes on other architectures in D4212.

We can overlook the non use of the extra options (as you said, following current implementation), but as it stands, it's not just unused, but impossible to use from a language point of view. That's why I proposed a textual format (for the ABI type), but this can be done later. Please add a FIXME to that regard.

cheers,
--renato

olista01 closed this revision.Jun 20 2014, 3:16 AM

Thanks, committed revision 211349.