[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
----------------
rahmanl wrote:
> jhenderson wrote:
> > Does this need extending?
> I interpreted your comment as we should remove it. Did you mean we should add a separate extension for this?
I was referring to the underline, which didn't match the modified header length sorry for the confusion! I'm happy either way with having just the "normal" title, or both mentioned in the header.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:61
+# CHECK-NEXT: ]
+## Check that the using the SHT_LLVM_BB_ADDR_MAP_V0 section type generates
+## the same result as the SHT_LLVM_BB_ADDR_MAP type with Version=0.
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:87
+# CHECK-NEXT: ]
+# CHECK-NEXT: BBAddrMap [
+# CHECK-NEXT:   Function {
----------------
This didn't occur to me until now, but it's unfortunate that we have to have duplicate check patterns and near-duplicate YAML to do the v0 comparison check. I believe we can avoid it as follows:
1) Have an additional YAML file that just describes the section, with the Type (and potentially Version) field parameterised.
2) Create two ELF objects from this YAML, one with each of the two section types, the newer type having an explicit Version 0.
3) Run llvm-readobj twice, to dump each of them individually.
4) Use the same check pattern for the pair of these invocations.
What do you think?


================
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
----------------
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).


================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:133-135
+      - Version:  0
+        Feature:  0x0
+        Address:  0x0
----------------
Any particular reason you have a double space between the colon and value here and below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346