This is an archive of the discontinued LLVM Phabricator instance.

[llvm][TargetParser] Create new Architecture base classes
AbandonedPublic

Authored by Stoorx on May 5 2023, 2:07 AM.

Details

Summary

The beginning of the huge refactor on Triple class.
The aim is to implement triple-related features in more OOP way.

Diff Detail

Event Timeline

Stoorx created this revision.May 5 2023, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 2:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Stoorx requested review of this revision.May 5 2023, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 2:07 AM
Stoorx added a comment.May 5 2023, 2:12 AM

It is pretty arguable changes, because they are made in more OOP "Java-way". Since that I would be glad to discuss some design of architecture of my solution.

This commit just adds new classes, but does NOT integrate them with llvm itself yet.

I am in doubt about correct list of reviewers. And maybe we can discuss this solution somewhere else.

The beginning of the huge refactor on Triple class.
The aim is to implement triple-related features in more OOP way.

Is there an RFC for this (sorry I often miss things on Discourse) ? Please can you include the link in the summary.

Stoorx added a comment.May 5 2023, 5:28 AM

I did not make RFC at Discourse. (Should I?)

I am newbie at LLVM project, so I do not fully understand how to contribute such major refactoring (and at first show my ideas to someone).

P.S. I read the policy again and find that I should make the page on the Discourse. Can anyone help me to do that in right way?

Stylistic comments, to help readers find the forest amid all the trees. :)
Probably should abbreviate Architecture as Arch everywhere, not just sometimes. It's a well-understood abbreviation.

llvm/include/llvm/TargetParser/Architecture/ArchitectureHelpers.h
8

LLVM style is to use header guards
(comment applies to all headers)

15

A final class with a deleted constructor and only static methods seems like a more complicated way to define a namespace; so please just use a namespace.

llvm/lib/TargetParser/Architecture/CMakeLists.txt
1

The project prefers to have RTTI off. Where do you need this?

llvm/unittests/TargetParser/Architecture/CMakeLists.txt
1

The project prefers to have RTTI off. Where do you need this?

mehdi_amini added inline comments.
llvm/include/llvm/TargetParser/Architecture/IDefaultFormatProvider.h
15

If you could document everything that is public (and non-trivial things in general), that would help. Right now reading the patch is a bit hard..

Stoorx abandoned this revision.May 6 2023, 10:10 AM
Pivnoy added a subscriber: Pivnoy.Jul 23 2023, 4:59 AM