[PATCH] D127165: [DirectX][MC] Add MC support for DXContainer

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:666
+  
+  // Get the section for the provided Section name
+  MCSectionDXContainer *getDXContainerSection(StringRef Section, SectionKind K);
----------------
nit: Other function comments start with 3 slashes


================
Comment at: llvm/include/llvm/MC/MCDXContainerStreamer.h:9
+//
+// Overrides MCObjectStreamer to disable all unnecessary features with stubs.
+//
----------------
Could we also describe what features this implementation *does* use and why it's needed for DXContainers?


================
Comment at: llvm/include/llvm/MC/MCDXContainerStreamer.h:35
+
+  bool emitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override {
+    return false;
----------------
Shouldn't one virtual function be defined in a cpp file? https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers

Separately, I'm concerned about unused argument warnings. (Also with other functions in this patch. If there are existing functions with named but unused arguments in the surrounding code, please disregard this comment.)


================
Comment at: llvm/include/llvm/MC/MCDXContainerWriter.h:25
+public:
+  virtual ~MCDXContainerTargetWriter() = default;
+
----------------
Also here


================
Comment at: llvm/include/llvm/MC/MCSectionDXContainer.h:33
+                            raw_ostream &OS,
+                            const MCExpr *Subsection) const override {}
+  bool useCodeAlign() const override { return false; }
----------------
Also here


================
Comment at: llvm/lib/MC/MCContext.cpp:847
+  // Do the lookup, if we have a hit, return it.
+  auto R = DXCUniquingMap.try_emplace(Section);
+  if (!R.second)
----------------
Can we use a more descriptive variable name? The uses like `R.first->second` don't help much wrt readability.

For example, what if we renamed `Section` -> `SectionName` and did something like:
```
auto ItInsertedPair = DXCUniquingMap.try_emplace(SectionName);
StringMapEntry<MCSectionDXContainer *> &Entry = *ItInsertedPair.first;

if (!ItInsertedPair.second)
  return Entry;

Entry.setValue(new (DXCAllocator.Allocate()) MCSectionDXContainer(SectionName, K, nullptr));
...

return Entry.getValue();
```

(pseudocode, I haven't actually tried it)


================
Comment at: llvm/lib/MC/MCContext.cpp:851
+
+  StringRef Name = R.first->first();
+  R.first->second =
----------------
Isn't this the same as the `Section` argument?


================
Comment at: llvm/lib/MC/MCDXContainerStreamer.cpp:23
+    bool RelaxAll) {
+  MCDXContainerStreamer *S = new MCDXContainerStreamer(
+      Context, std::move(MAB), std::move(OW), std::move(CE));
----------------
ubernit: `auto *S`, since the type is obvious


================
Comment at: llvm/lib/MC/MCDXContainerWriter.cpp:50
+  // Start the file size as the header plus the size of the part offsets.
+  llvm::SmallVector<uint64_t, 16> PartOffsets;
+  uint64_t PartOffset = 0;
----------------
How did you pick this constant?


================
Comment at: llvm/lib/MC/MCDXContainerWriter.cpp:75
+  // Write the header
+  W.write<char>({'D', 'X', 'B', 'C'});
+  // Hash 16-bytes of 0's
----------------
nit: This is probably pedantic, but I think technically characters don't have to use ASCII on all platforms and it would be safer to use exact hex values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127165