DXContainer files resemble traditional object files in that they are
comprised of parts which resemble sections. Adding DXContainer as an
object file format in the MC layer will allow emitting DXContainer
objects through the normal object emission pipeline.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/MC/MCContext.h | ||
---|---|---|
666 | nit: Other function comments start with 3 slashes | |
llvm/include/llvm/MC/MCDXContainerStreamer.h | ||
10 | Could we also describe what features this implementation *does* use and why it's needed for DXContainers? | |
36 | 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.) | |
llvm/include/llvm/MC/MCDXContainerWriter.h | ||
26 | Also here | |
llvm/include/llvm/MC/MCSectionDXContainer.h | ||
34 | Also here | |
llvm/lib/MC/MCContext.cpp | ||
847 | 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) | |
851 | Isn't this the same as the Section argument? | |
llvm/lib/MC/MCDXContainerStreamer.cpp | ||
24 | ubernit: auto *S, since the type is obvious | |
llvm/lib/MC/MCDXContainerWriter.cpp | ||
51 | How did you pick this constant? | |
76 | 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. |
Updates coming.
llvm/lib/MC/MCContext.cpp | ||
---|---|---|
851 | 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?). | |
llvm/lib/MC/MCDXContainerWriter.cpp | ||
51 | 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. | |
76 | 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. |
llvm/lib/MC/MCDXContainerWriter.cpp | ||
---|---|---|
76 | Gah! UTF-8 character literals are C++17 :( Since we do have other places (like the ELF ZILB constant) where we assume character literals are ASCII, and I'm completely unaware of any non-ASCII platform that supports enough memory to build or run LLVM, I'm going to prefer to keep this as characters. |
I don't know much about DX but this looks like object emission to me. :)
I guess any problem from these changes will only show up in DX lowering, so not very high risk to other targets...
llvm/lib/MC/MCObjectFileInfo.cpp | ||
---|---|---|
1015 | What about the DXIL section, do you need to initialise it here, too? |
llvm/lib/MC/MCObjectFileInfo.cpp | ||
---|---|---|
1015 | I treat the DXIL section as a data section the same way other backends emit bitcode into sections, so it gets initialized when the global variable emission gets triggered. |
llvm/include/llvm/MC/MCDXContainerWriter.h | ||
---|---|---|
21 | delete blank line |
Updating based on feedback from @MaskRay.
Working on additional documentation as well.
nit: Other function comments start with 3 slashes