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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
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"
----------------
asb wrote:
> A number of these seem to be unused.
Remove unused including files.


================
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;
----------------
asb wrote:
> 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.
I think this should be reasonable default ABI EFLAGs setting before introducing -target-abi flag. OK, we could add the logic once -target-abi are supported.


================
Comment at: lib/Target/RISCV/RISCVTargetStreamer.h:1
+//===-- RISCVTargetStreamer.h - RISCV Target Streamer ----------*- C++ -*--===//
+//
----------------
asb wrote:
> Shouldn't this file be in lib/Target/RISCV/MCTargetDesc?
Move RISCVTargetStreamer.h to 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"
----------------
asb wrote:
> Most of these seem to be unused.
Remove unused including files.


================
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
----------------
asb wrote:
> Doesn't this really belong as a test/MC/RISCV test? (obviously modified to use llvm-mc instead).
Move the test case to test/MC/RISCV and rename to elf-flags.s


Repository:
  rL LLVM

https://reviews.llvm.org/D41658