[PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai marked 3 inline comments as done.
HsiangKai added inline comments.


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:28
+enum AttrType {
+  // Rest correspond to ELF/.riscv.attributes
+  Stack_align = 4,
----------------
jhenderson wrote:
> Rest of what?
Originally, there are three common types before architecture-specific attributes, File, Section, Symbol. These three attribute types have been moved to ELFAttributes.h. I think it is indeed confusing here. I will modify this comment.


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:29-34
+  Stack_align = 4,
+  Arch = 5,
+  Unaligned_access = 6,
+  Priv_spec = 8,
+  Priv_spec_minor = 10,
+  Priv_spec_revision = 12,
----------------
jhenderson wrote:
> These names are not in LLVM style. Are they matching part of the specification? If not, please change them.
These are some definitions in RISC-V ELF specification. I looked up some ELF definitions in llvm/include/llvm/BinaryFormat/ELF.h. I think it should be fine to include underscore in the names. So, I will capitalize all these names and keep underscores.


================
Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:14
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags = {
+    {ARMBuildAttrs::File, "Tag_File"},
+    {ARMBuildAttrs::Section, "Tag_Section"},
----------------
jhenderson wrote:
> The formatting changes here seem unrelated so shouldn't be made with this change, right?
They are formatted by clang-format. Actually, I am a little bit confused whether to apply clang-format on unrelated code or not. I will restore them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74023/new/

https://reviews.llvm.org/D74023