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

[Chris Bieneman via Phabricator via llvm-commits]

beanz marked 7 inline comments as done.
beanz added a comment.

Updates coming.



================
Comment at: llvm/lib/MC/MCContext.cpp:851
+
+  StringRef Name = R.first->first();
+  R.first->second =
----------------
kuhar wrote:
> Isn't this the same as the `Section` argument?
It should be the same text, but is likely a different address. The address from the StringMap is guaranteed to remain valid through the lifetime of the object writer, while the input may come from an IR construct that has a different lifetime (aren't unowned StringRefs fun?).


================
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;
----------------
kuhar wrote:
> How did you pick this constant?
DXContainers generally end up with 7-10 parts. I picked the next power of two above that range so that it would likely all fit in the space. I'll elaborate in a comment.


================
Comment at: llvm/lib/MC/MCDXContainerWriter.cpp:75
+  // Write the header
+  W.write<char>({'D', 'X', 'B', 'C'});
+  // Hash 16-bytes of 0's
----------------
kuhar wrote:
> 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.
So... I will concede that technically someone could attempt to compile LLVM for a non-ASCII platform, and this _could_ be a problem. If someone were to use this on a non-ASCII platform this would be the least of our problems because we do a lot of string-compares on binary data, and we generally encode section names and other "strings" as ASCII.

How about a happy middle ground, I can make these explicit UTF-8 constants. I prefer the readability of characters rather than needing to keep an ASCII chart handy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127165