[PATCH] D116463: [SPIRV 4/6] Add target lowering, TargetMachine and AsmPrinter

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added a comment.

@MaskRay, thanks for your comments. I'll fix the issues later today.



================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:52
+    unsigned i = 0;
+    for (const auto &Arg : F.args()) {
+      // Currently formal args should use single registers.
----------------
MaskRay wrote:
> There is a Clang -Wunused-variable warning.
Yes, but Arg is used in the next [[ https://reviews.llvm.org/D116464 | 5th patch ]] so the warning disappears. I guess it's acceptable to leave this until the 5th patch application, isn't it?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVTargetObjectFile.h:31
+                                   Align &Alignment) const override {
+    return TextSection;
+  }
----------------
MaskRay wrote:
> The 3 functions need a comment. Using a non-data section seems weird to readers.
Ok, I'll add a comment saying that all contents of SPIRV file are instructions, so we put everything in a text section.


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

https://reviews.llvm.org/D116463