[RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer

[shiva0217]

Summary

Encode RISCV specific ELF e_flag EF_RISCV_RVC to RISCV Binary

[asb]

Thanks Shiva - comments inline

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

15–25

A number of these seem to be unused.

45–48

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.

lib/Target/RISCV/RISCVTargetStreamer.h

1 ↗

(On Diff #128396)

Shouldn't this file be in lib/Target/RISCV/MCTargetDesc?

13–17 ↗

(On Diff #128396)

Most of these seem to be unused.

test/CodeGen/RISCV/elf-header.ll

1 ↗

(On Diff #128396)

Doesn't this really belong as a test/MC/RISCV test? (obviously modified to use llvm-mc instead).

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

15–25

Remove unused including files.

45–48

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.

lib/Target/RISCV/RISCVTargetStreamer.h

1 ↗

(On Diff #128396)

Move RISCVTargetStreamer.h to lib/Target/RISCV/MCTargetDesc

13–17 ↗

(On Diff #128396)

Remove unused including files.

test/CodeGen/RISCV/elf-header.ll

1 ↗

(On Diff #128396)

Move the test case to test/MC/RISCV and rename to elf-flags.s

[asb]

With a couple of minor nits, looks good to me. Thanks as always Shiva

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

23

Nitpick: full-stop after comment.

Does the comment add value? Maybe just delete it.

test/MC/RISCV/elf-flags.s

2

Need to set the target for llvm-mc.

Should probably have RUN lines for both riscv32 and riscv64 as many of the other test/MC/RISCV tests do.

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

23

Remove the redundant comment

test/MC/RISCV/elf-flags.s

2

Run the test case for riscv32 and riscv64 target.

[asb]

Hi Shiva. I was just looking at applying this and the STI field is triggering a compiler warning as it's not used: it's initialised in the RISCVTargetELFStreamer constructor, but isn't actually read from anywhere.

The test could be modified slightly - given that we know all defined ELF flags for RISC-V are shared between RV32 and RV64 we can share check line, and we could also check that the RVC flag isn't improperly being when not targeting RVC. How about writing it like this:

# RUN: llvm-mc -triple=riscv32 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
# RUN: llvm-mc -triple=riscv64 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
# RUN: llvm-mc -triple=riscv32 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s
# RUN: llvm-mc -triple=riscv64 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s

# CHECK-RVI:       Flags [ (0x0)
# CHECK-RVI-NEXT:  ]

# CHECK-RVIC:       Flags [ (0x1)
# CHECK-RVIC-NEXT:    EF_RISCV_RVC (0x1)
# CHECK-RVIC-NEXT:  ]

nop

[asb]

My suggestion would be to remove the MCSubtarget field (it can be reintroduced when future changes need it), but obviously keep it as an argument to the constructor where it is used.

[shiva0217]

Remove MCSubtargetInfo field and refine the test case as @asb suggest.

[apazos]

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

74

Should'nt we check for ELF format, that is the only one supported now, but may change later?

const Triple &TT = STI.getTargetTriple();

if (TT.isOSBinFormatELF())
  return new RISCVTargetELFStreamer(S);
return nullptr;

89

Reading MCStreamer.h I understand when we define a target streamer we need to define two streamers, ELF and Asm. Is it how you interpreted?

/ Target specific streamer interface. This is used so that targets can
/ implement support for target specific assembly directives.
/
/ If target foo wants to use this, it should implement 3 classes:
/ * FooTargetStreamer : public MCTargetStreamer
/ * FooTargetAsmStreamer : public FooTargetStreamer
/// * FooTargetELFStreamer : public FooTargetStreamer

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

42

Shouldn't it be return static_cast<RISCVELFStreamer &>(Streamer);

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

74

Add ELF format checking before new RISCVTargetELFStreamer.

89

To my understanding, AsmStreamer is used to emit target specific assembly directives,
E.g. ARM emit .fpu directive by AsmStreamer and encode the information to a section by ELFStreamer. It seems that RISCV GCC will not emit directive to specify target feature(E.g. C EXT ISA), so I define ELFStreamer first. I think we could introduce the AsmStreamer in the future patch when we need to emit assembly directives.

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

42

We didn't define RISCVELFStreamer class for emitting e_flags. It seems that we could introduce the class in the feature patch?

[asb]

clang-format shows me a few minor tweaks:

• RISCVELFStreamer.h include should come first in RISCVELFStreamer.cpp
• RISCVMCAsmInfo.h include is now improperly sorted in RISCVMCTargetDesc.cpp
• RISCVTargetStreamer constructor can fit on one line in 80 chars RISCVTargetStreamer::RISCVTargetStreamer(MCStreamer &S) : MCTargetStreamer(S) {}

Otherwise it looks good to me. I can't see any hard dependency on a custom RISCVAsmStreamer (though it's likely we'll want to add it later). Ana - are you happy for this patch to be committed as-is?

[apazos]

Thanks, this looks fine to me. You can merge it. I will rebase my change on top of this.

[apazos]

Also depends on the WIP in https://reviews.llvm.org/D42228

[asb]

Hi Shiva, why don't you follow the instructions at http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and request commit access? Many many thanks again for all of your contributions to the RISC-V backend so far.

[shiva0217]

Hi Alex, thanks for the advice. Hi Ana, I have committed the patch to the trunk.

[asb]

Hi Shiva. In the future it's worth nothing that if you include the review url in the commit, then Phabricator automatically picks it up.

e.g. if you included this line then phabricator would automatically close this review:
Differential Revision: https://reviews.llvm.org/D41658

It looks like you missed the minor sorting/whitespace fixes I suggested https://reviews.llvm.org/D41658#978491. If you have time to commit a follow-up cleanup I'd appreciate it (just committing directly for this cleanup is fine). Thanks!

[asb]

Sorry, my mistake - the committed version did include the suggested changes.

[shiva0217]

Hi Alex. Thanks for your tips, I'll remember that.

[shiva0217]

Closed by commit rL323507: [RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer

lib/Target/RISCV/MCTargetDesc/CMakeLists.txt

 add_llvm_library(LLVMRISCVDesc
   RISCVAsmBackend.cpp
   RISCVELFObjectWriter.cpp
   RISCVMCAsmInfo.cpp
   RISCVMCCodeEmitter.cpp
   RISCVMCExpr.cpp
   RISCVMCTargetDesc.cpp
+  RISCVTargetStreamer.cpp
+  RISCVELFStreamer.cpp
 )

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h

+//===-- RISCVELFStreamer.h - RISCV ELF Target Streamer ---------*- C++ -*--===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_RISCV_RISCVELFSTREAMER_H
+#define LLVM_LIB_TARGET_RISCV_RISCVELFSTREAMER_H
+
+#include "RISCVTargetStreamer.h"
+#include "llvm/MC/MCELFStreamer.h"
+
+namespace llvm {
+
+class RISCVTargetELFStreamer : public RISCVTargetStreamer {
+public:
+  MCELFStreamer &getStreamer();
+  RISCVTargetELFStreamer(MCStreamer &S, const MCSubtargetInfo &STI);
+};
+}
+#endif

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

+//===-- RISCVELFStreamer.cpp - RISCV ELF Target Streamer Methods ----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides RISCV specific target streamer methods.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCVMCTargetDesc.h"
+#include "RISCVELFStreamer.h"
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+
+using namespace llvm;
+
+// This part is for ELF object output.
+RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
+                                               const MCSubtargetInfo &STI)
+    : RISCVTargetStreamer(S) {
+  MCAssembler &MCA = getStreamer().getAssembler();
+
+  const FeatureBitset &Features = STI.getFeatureBits();
+
+  unsigned EFlags = MCA.getELFHeaderEFlags();
+
+  if (Features[RISCV::FeatureStdExtC])
+    EFlags |= ELF::EF_RISCV_RVC;
+
+  MCA.setELFHeaderEFlags(EFlags);
+}
+
+MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
+  return static_cast<MCELFStreamer &>(Streamer);
+}

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 //===-- RISCVMCTargetDesc.cpp - RISCV Target Descriptions -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// This file provides RISCV-specific target descriptions.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCTargetDesc.h"
 #include "InstPrinter/RISCVInstPrinter.h"
 #include "RISCVMCAsmInfo.h"
+#include "RISCVELFStreamer.h"
+#include "RISCVTargetStreamer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"

 static MCInstPrinter *createRISCVMCInstPrinter(const Triple &T,
                                                unsigned SyntaxVariant,
                                                const MCAsmInfo &MAI,
                                                const MCInstrInfo &MII,
                                                const MCRegisterInfo &MRI) {
   return new RISCVInstPrinter(MAI, MII, MRI);
 }
 
+static MCTargetStreamer *
+createRISCVObjectTargetStreamer(MCStreamer &S, const MCSubtargetInfo &STI) {
+  const Triple &TT = STI.getTargetTriple();
+  if (TT.isOSBinFormatELF())
+    return new RISCVTargetELFStreamer(S, STI);
+  return new RISCVTargetStreamer(S);
+}
+
 extern "C" void LLVMInitializeRISCVTargetMC() {
   for (Target *T : {&getTheRISCV32Target(), &getTheRISCV64Target()}) {
     TargetRegistry::RegisterMCAsmInfo(*T, createRISCVMCAsmInfo);
     TargetRegistry::RegisterMCInstrInfo(*T, createRISCVMCInstrInfo);
     TargetRegistry::RegisterMCRegInfo(*T, createRISCVMCRegisterInfo);
     TargetRegistry::RegisterMCAsmBackend(*T, createRISCVAsmBackend);
     TargetRegistry::RegisterMCCodeEmitter(*T, createRISCVMCCodeEmitter);
     TargetRegistry::RegisterMCInstPrinter(*T, createRISCVMCInstPrinter);
     TargetRegistry::RegisterMCSubtargetInfo(*T, createRISCVMCSubtargetInfo);
+    TargetRegistry::RegisterObjectTargetStreamer(
+        *T, createRISCVObjectTargetStreamer);
   }
 }

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

+//===-- RISCVTargetStreamer.h - RISCV Target Streamer ----------*- C++ -*--===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_RISCV_RISCVTARGETSTREAMER_H
+#define LLVM_LIB_TARGET_RISCV_RISCVTARGETSTREAMER_H
+
+#include "llvm/MC/MCStreamer.h"
+
+namespace llvm {
+
+class RISCVTargetStreamer : public MCTargetStreamer {
+public:
+  RISCVTargetStreamer(MCStreamer &S);
+};
+}
+#endif

lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

+//===-- RISCVTargetStreamer.cpp - RISCV Target Streamer Methods -----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides RISCV specific target streamer methods.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCVTargetStreamer.h"
+
+using namespace llvm;
+
+RISCVTargetStreamer::RISCVTargetStreamer(MCStreamer &S)
+    : MCTargetStreamer(S) {}

test/MC/RISCV/elf-flags.s

+# RUN: llvm-mc -triple=riscv32 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
+# RUN: llvm-mc -triple=riscv64 -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVI %s
+# RUN: llvm-mc -triple=riscv32 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s
+# RUN: llvm-mc -triple=riscv64 -mattr=+c -filetype=obj < %s | llvm-readobj -file-headers - | FileCheck -check-prefixes=CHECK-RVIC %s
+
+# CHECK-RVI:       Flags [ (0x0)
+# CHECK-RVI-NEXT:  ]
+
+# CHECK-RVIC:       Flags [ (0x1)
+# CHECK-RVIC-NEXT:    EF_RISCV_RVC (0x1)
+# CHECK-RVIC-NEXT:  ]
+
+nop