[PATCH] D121346: [Propeller] Encode address offsets of basic blocks relative to the end of the previous basic blocks.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/Extensions.rst:399
+``SHT_LLVM_BB_ADDR_MAP`` and ``SHT_LLVM_BB_ADDR_MAP_V0`` Sections (basic block address map)
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 This section stores the binary address of basic blocks along with other related
----------------
Does this need extending?


================
Comment at: llvm/docs/Extensions.rst:456
 
-``.cv_file`` Directive
+``.cv_file`` Directive 
 ^^^^^^^^^^^^^^^^^^^^^^
----------------
Nit: looks like this line has gained some trailing whitespace somehow.


================
Comment at: llvm/lib/Object/ELF.cpp:669-672
+      if (Version > 1) {
+        return createError("unsupported LLVM_BB_ADDR_MAP version: " +
+                           Twine(static_cast<int>(Version)));
+      }
----------------
Nit: no need for braces here.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels.ll:49-50
 ; NOUNIQ:		.section	.llvm_bb_addr_map,"o", at llvm_bb_addr_map,.text,unique,1
+; CHECK-NEXT:   .byte   1
+; CHECK-NEXT:   .byte   0
 ; CHECK-NEXT:	.quad	.Lfunc_begin0
----------------
It would be good if these could have comments in the asm indicating what they represent (i.e. version and feature), for those not familiar with the format.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:193
+    Type:   SHT_LLVM_BB_ADDR_MAP_V0
+    Link:   .text.baz
+    Entries:
----------------
I wonder if it would be better to link against the same section? This would allow you to compare the differences more easily.


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:109
 # MULTI-NEXT:     Entries:
-## The 'Address' field is omitted when it's zero.
+## Fields 'Address', 'Version', and 'Feature' are omitted when theyr are zero.
 # MULTI-NEXT:       - BBEntries:
----------------
Typo


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:129
     Entries:
-## Check that obj2yaml does not emit the Address field when it's zero.
-      - Address:   0x0
+## Check that obj2yaml does not emit the Address, Version, and Feature fields when it's zero.
+      - Version: 0
----------------
I actually think the Version field should be mandatory. It seems odd to pin the default to the oldest version, but we also shouldn't have it change when a new version is added as otherwise it'll cause existing YAML to change behaviour.


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:132
+        Feature: 0x0
+        Address:   0x0
         BBEntries:
----------------
Nit: let's line things up.


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:181
+# V0-NEXT:             Metadata:         0x3
+#
+--- !ELF
----------------
Nit


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:897
+    if (Shdr->sh_type == ELF::SHT_LLVM_BB_ADDR_MAP) {
+      Version = Data.getU8(Cur);
+      Feature = Data.getU8(Cur);
----------------
We probably should emit an error for unsupported versions. The file format may change in a future version such that the existing parsing will break in nasty ways. Same probably goes for llvm-readobj.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346