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

Tue Jun 21 11:01:01 PDT 2022

rahmanl added a comment.

Thanks for the review @jhenderson

================
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
----------------
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?

================
Comment at: llvm/test/tools/obj2yaml/ELF/bb-addr-map.yaml:133-135
+      - Version:  0
+        Feature:  0x0
+        Address:  0x0
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121346