[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/lib/Object/ELF.cpp:670-671
+      if (Version > 1)
+        return createError("unsupported LLVM_BB_ADDR_MAP version: " +
+                           Twine(static_cast<int>(Version)));
+      Data.getU8(Cur); // Feature byte
----------------
Test case?

Also, the type is "SHT_LLVM_BB_ADDR_MAP", so probably wants to include the SHT_ too, to match (and be consistent with other error messages)


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll:13
 ; CHECK:		.section .llvm_bb_addr_map,"o", at llvm_bb_addr_map,.text._Z3barv{{$}}
-; CHECK-NEXT:		.quad [[BAR_BEGIN]]
+; CHECK:		.quad [[BAR_BEGIN]]
 
----------------
Should we instead be including the version etc bytes? (I don't mind, just trying to understand the thought process)


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels-functions-sections.ll:35
 ; CHECK:		.section .llvm_bb_addr_map,"Go", at llvm_bb_addr_map,_Z4fooTIiET_v,comdat,.text._Z4fooTIiET_v{{$}}
-; CHECK-NEXT:		.quad [[FOOCOMDAT_BEGIN]]
+; CHECK:		.quad [[FOOCOMDAT_BEGIN]]     # function address
----------------
If you're adding the comment here, I'd also add it to the other cases above (plus it makes it more robust, since it reduces the chance of spurious matches)


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:150
+
+
+## Check that using the SHT_LLVM_BB_ADDR_MAP_V0 section type generates the same
----------------
Nit: double blank line.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:158
+# RUN: yaml2obj --docnum=2 %s -DVERSION=255 -DSECTION_TYPE=SHT_LLVM_BB_ADDR_MAP_V0 -o %t2.type0
+# RUN: llvm-readobj %t2.type0 --bb-addr-map 2>&1 | FileCheck %s --check-prefixes=V0
+
----------------
Super Nit: here and throughout, --check-prefixes -> --check-prefix when there's only one prefix to check (optional though - if you prefer to leave as-is, that's fine).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:186
+# V0-NEXT:   }
+#
+
----------------
Nit: spurious extra line?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:196
+    Entries:
+# Version field is required but ignored for SHT_LLVM_BB_ADDR_MAP_V0.
+      - Version: 255
----------------
rahmanl wrote:
> jhenderson wrote:
> > Ah, that's an unfortunate side-effect. I think we should aim to avoid it somehow. About the best idea I have for this is to use different struct types in the ELFYAML code for SHT_LLVM_BB_ADDR_MAP_V0 entries and those in SHT_LLVM_BB_ADDR_MAP sections. This also means you can't set Features when it doesn't make sense (which is a good thing).
> The problem is I would have to add alternative structs for `BBAddrMapSection` and `BBAddrMapEntry` and also define new mapping functions and `writeSectionContent` (with mostly identical code) for the `SHT_LLVM_BB_ADDR_MAP_V0` type. We should be able to fully deprecate `SHT_LLVM_BB_ADDR_MAP_V0` in a few months. So maybe this test won't stay around for too long. Of course, future versions will still use the same `SHT_LLVM_BB_ADDR_MAP` section type and therefore, new YAML fields will be optional (even if they are required for the new versions). So we won't have a major issue. Can I keep it as is?
Yeah, leave as-is. Thanks for the explanation.


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:133-135
+      - Version:  0
+        Feature:  0x0
+        Address:  0x0
----------------
rahmanl wrote:
> jhenderson wrote:
> > Any particular reason you have a double space between the colon and value here and below?
> It should be 3 spaces because of `NumBlocks` being used sometime. Aligned the YAML keys in this test more carefully.
FWIW, I only align within the individual block, so here, I'd align with only the single space, and then use 3 spaces where NumBlocks is present. I don't care really though, as long as the spacing doesn't get excessive (at which point it can make readability an issue).


================
Comment at: llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml:79-81
   - Name:    '.llvm_bb_addr_map (3)'
     Type:    SHT_LLVM_BB_ADDR_MAP
+    Size: 8
----------------
Nit: these should line up.


================
Comment at: llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml:136
     Content: [[CONTENT=<none>]]
-    Size:    [[SIZE=<none>]]
+    Size: [[SIZE=<none>]]
+
----------------
Nit: this should line up.


================
Comment at: llvm/test/tools/yaml2obj/ELF/bb-addr-map.yaml:142
+# INVALID-VERSION: unsupported SHT_LLVM_BB_ADDR_MAP version: 2
+--- !ELF
+FileHeader:
----------------
Nit: for consistent formatting, add a blank line before the YAML.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346