[PATCH] D41658: [RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Shiva - comments inline



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:14-24
+#include "RISCVMCExpr.h"
+#include "RISCVMCTargetDesc.h"
+#include "RISCVTargetStreamer.h"
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCSectionELF.h"
+#include "llvm/MC/MCSubtargetInfo.h"
----------------
A number of these seem to be unused.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:44-47
+  if (Features[RISCV::FeatureStdExtD])
+    EFlags |= ELF::EF_RISCV_FLOAT_ABI_DOUBLE;
+  else if (Features[RISCV::FeatureStdExtF])
+    EFlags |= ELF::EF_RISCV_FLOAT_ABI_SINGLE;
----------------
I don't think this logic is doing what we want. The ABI EFLAGs should be determined by the target ABI rather than the target architecture. I'd suggest removing the FLOAT_ABI code and tests, and we can add this logic once alternative -target-abi are supported.


================
Comment at: lib/Target/RISCV/RISCVTargetStreamer.h:1
+//===-- RISCVTargetStreamer.h - RISCV Target Streamer ----------*- C++ -*--===//
+//
----------------
Shouldn't this file be in lib/Target/RISCV/MCTargetDesc?


================
Comment at: lib/Target/RISCV/RISCVTargetStreamer.h:13-17
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/MC/MCELFStreamer.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCStreamer.h"
----------------
Most of these seem to be unused.


================
Comment at: test/CodeGen/RISCV/elf-header.ll:1
+; RUN: llc -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck --check-prefix=ExtC %s
+; RUN: llc -mattr=+d -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck --check-prefix=ExtD %s
----------------
Doesn't this really belong as a test/MC/RISCV test? (obviously modified to use llvm-mc instead).


Repository:
  rL LLVM

https://reviews.llvm.org/D41658