This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Move PropertyAnnotationNames into a cc file.
ClosedPublic

Authored by jlebar on Dec 9 2016, 3:57 PM.

Details

Summary

Previously they were defined as a 2D char array in a header file. This
is kind of overkill -- we can let the linker lay out these strings
however it pleases. While we're at it, we might as well just inline
these constants where they're used, as each of them is used only once.

Event Timeline

jlebar updated this revision to Diff 80966.Dec 9 2016, 3:57 PM
jlebar retitled this revision from to [NVPTX] Move PropertyAnnotationNames into a cc file..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added a subscriber: llvm-commits.
tra added inline comments.Dec 14 2016, 1:39 PM
llvm/lib/Target/NVPTX/MCTargetDesc/NVPTXBaseInfo.cpp
14

Do we really need a separate file for this? Why not just put it into NVPTXUtilities.cpp where it's used. For all practical purposes we can even make it static there as there are no other users.

jlebar updated this revision to Diff 81473.Dec 14 2016, 2:28 PM

Inline all the constants.

Sure, let's just get rid of these constants altogether. Simple is better than complex.

jlebar updated this object.Dec 14 2016, 2:28 PM
jlebar updated this revision to Diff 81474.Dec 14 2016, 2:29 PM

Revert changes to CMakeLists.txt.

jlebar updated this revision to Diff 81477.Dec 14 2016, 2:34 PM

...and remove empty file I didn't see in the git diff.

tra accepted this revision.Dec 14 2016, 2:35 PM
tra edited edge metadata.

Nice!

llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
270–271

if (findOneNVVMAnnotation()) ... ? Here and in getAlign() below?

This revision is now accepted and ready to land.Dec 14 2016, 2:35 PM
jlebar added inline comments.Dec 14 2016, 2:40 PM
llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
270–271

Ugh, I accidentally merged two patches, the one moving this file into namespace llvm and the other getting rid of the constants.

I guess what's done is done. I'll send another patch for the retval thing, though, as this is already bigger than I would like.

This revision was automatically updated to reflect the committed changes.