Page MenuHomePhabricator

XCore target: Add TypeString meta data to IR output.
ClosedPublic

Authored by robertlytton on Mar 3 2014, 10:42 AM.

Details

Summary

This includes the addition of the virtual function:

virtual void TargetCodeGenInfo::EmitTargetMD(const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &M) const { }

Richard Osborne to review from XCore perspective
Also need review from Clang viewpoint re: EmitTargetMD - Rafael, can you?
Also the code correctness - Richard again.

Thank you

Robert

Diff Detail

Event Timeline

There is a problem when running the test-suite,
I'll investigate, and add patch to code/tests as necessary.

Please use the current naming convesion, so emitTargetMD. Eric has
worked more with medatata (for debug info) and probably has a better
idea than I if this is the correct way to plug it.

Can you describe a bit what this metadata is?

What are you trying to do here?

Hi Eric,

The XCore ABI includes a type information section that communicates symbol type information to the linker.
The linker uses this information to verify safety/correctness of things such as array bound and pointers etc.

This type information (TypeString) is emitted into meta data for all global symbols - definitions, declarations, functions & variables.
The TypeString carries type, qualifier, name, size & value details - as verified in xcore-stringtype.c

Currently meta data seems the best way to get this information to the back-end and hence the linker.
It seems the IR could encode this information as attributes for functions definitions & declaration - but global variables don't have attributes.

The addition of emitTargetMD() is because SetTargetAttributes() only visits function definitions.
Is there a better hook or better mechanism?

Thank you.

Robert

robertlytton updated this revision to Unknown Object (????).Mar 10 2014, 5:56 AM

Add check for null Decl pointer.
(Will be null for function declarations added by clang viz exception handling support)

robertlytton updated this revision to Unknown Object (????).Mar 10 2014, 8:35 AM

Add handling of anonymous structures, unions and enums.
Also reduce the expansion of recursive record types.

robertlytton updated this revision to Unknown Object (????).Mar 11 2014, 2:05 AM

fix comment in test file.

This needs the changes that Rafael mentioned as well as needing the format that you're emitting documented.

Few other comments inline for now.

test/CodeGen/xcore-stringtype.c
7

"their"

Also why 33? Also what's supported/unsupported? It's not obvious from the comment in the testcase and would require someone knowing the entire scheme to debug.

36

It would be nice if the code in the testcase followed the llvm coding standard.

robertlytton updated this revision to Unknown Object (????).Apr 1 2014, 4:35 AM

Renamed EmitTargetMD emitTargetMD.
Added explanatory comment to GetTypeString().
Added explanatory comment, lowered initial function capitals and reduced line lengths in test/CodeGen/xcore-stringtype.c

Some more inline comments...

lib/CodeGen/TargetInfo.cpp
6085
6274

I'd say that since this is an odd vendor thing that we might want to just include the format in the text that describes it?

On the other hand if xmos isn't in business anymore we could just delete the port immediately for lack of documentation? :)

robertlytton updated this revision to Unknown Object (????).Apr 3 2014, 8:57 AM

Hi Eric,

I feel suitably embarrassed and have reread the coding standard again as penance.
Functions are now initial lower case.
I have also used auto for iterator declarations.

The format details for the TypeString metadata is quite long and I would agree with your sentiments.

Robert

Added a few more comments inline.

Thanks!

lib/CodeGen/TargetInfo.cpp
6084

What's going on with IncompleteUsedCount? I don't quite get it.

6218

Comment for the function.

6222

Sentences begin with a capital letter :)

6228

Comment for the function.

6247

Do you check the return value of this? Also comment.

6257

Comment on what's going on here. Why does "str" check the state?

6332

You can check whether the record is a union without passing it down as a bool.

6362

Why not check Success before the line above it?

6364

Sorting union encodings and not struct ones? Weird.

robertlytton updated this revision to Unknown Object (????).Apr 15 2014, 3:20 AM

Hi Eric,

I've extended the comments as suggested.
I have also extended the algorithm to be able to cache recursive types for use when they are non-members.
The interface into the cache has also be tidied.

Thank you for your input.

Robert

robertlytton updated this revision to Unknown Object (????).Apr 15 2014, 3:25 AM

(remove stray '/' that got accidentally added to diff)

robertlytton updated this revision to Unknown Object (????).Apr 16 2014, 2:35 AM

I added a bug with the change in the algorithm.
Here is an extra test and corrective code.

Needs block comments for all of the static functions. Otherwise I don't see anything terrible.

robertlytton updated this revision to Unknown Object (????).Apr 17 2014, 9:34 AM

Added comments to static functions.
cheers.
Good to go?

robertlytton updated this revision to Diff 8812.EditedApr 24 2014, 10:05 AM
robertlytton edited edge metadata.

Just spotted that a object had a constructor as its type rather than the class.

friedgold added inline comments.Apr 25 2014, 9:43 AM
lib/CodeGen/TargetInfo.cpp
6117

TypesString -> TypeString

6118

A found this a bit unclear. How about:

An incomplete TypesString that has been used in a Recursive type encoding

6154

The members of the Entry struct could do with comments. What does Swapped represent?

6163

Could you rename this to have lookup or find in the name - it isn't clear what the method does from the name.

6174

& should be next to the variable name.

6271

& should be next to the variable name.

6294

What's the difference between self recusive typestrings and recursive typestrings?

robertlytton edited edge metadata.

Hi Richard,
I've reworked comments etc based on you input.
There is no difference between 'self recusive typestrings' and 'recursive typestrings'.
I've renamed the offending variables.

friedgold accepted this revision.May 1 2014, 3:39 AM
friedgold edited edge metadata.

LGTM

robertlytton closed this revision.May 9 2014, 3:34 AM