[RISCV] writeNopData support generate c.nop

[shiva0217]

Summary

The assembler will generate nop for alignment. For the riscv target support C extension, it may need to generate c.nop to fix up alignment.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

37 ↗

(On Diff #126887)

Is there any way we can avoid spreading parsing logic throughout the backend? Can we get hold of an MCSubtargetInfo and check for STI.getFeatureBits()[RISCV::FeatureStdExtC]? The change to Triple.cpp then wouldn't be necessary in this patch (though might be something we want to look at, to make arguments more uniform between clang and the llvm tools).

103 ↗

(On Diff #126887)

I think it's kind of confusing define i here, then using it below, but also shadowing it with a new i in the NumOps loop. Would it be clearer to just calculate Nop16Count and Nop32Count, then decrement those values directly?

111 ↗

(On Diff #126887)

Indentation.

[shiva0217]

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

37 ↗

(On Diff #126887)

The latest revision will:

1. Add parseRISCVTriple to get target feature by parsing triple for the backend.
2. Add MCSubtargetInfo in RISCVAsmBackend class and using STI.getFeatureBits()[RISCV::FeatureStdExtC] to check target feature.
3. The STI initial by parsing triple, if we remove the change in Triple.cpp, triple with suffix imac will not allow, and then STI can't get target feature.

103 ↗

(On Diff #126887)

Yes, define Nop32Count and Nop16Count would be more readable.
Thanks.

[asb]

Creating a TgtMCSubtargetInfo without passing CPUName and FeatureString is what ARM does in ARMAsmBackend, but it seems pretty confusing. The fact that the desired behaviour doesn't work with -triple riscv32 -mattr=+c indicates to me that something is wrong.

D20830 added an MCSubtargetInfo parameter to relaxInstruction. I could see that the same justification could be used to add MCSubtargetInfo to writeNopData.

Does anyone else have views on this? It's been a while since I stepped through the TargetRegistry code - if feasible, having a properly initialised RISCVMCSubtargetInfo (with cpustring and featurestring set correctly) as a field of RISCVAsmBackend seems ideal, but from an initial look it might be difficult.

[shiva0217]

Hi Alex, I think https://reviews.llvm.org/D41349 is the right thing to do, then we could get correct MCSubtargetInfo by -mattr in MCAsmBackend class, thanks for your work.
We could remove the parsing logic in this patch once https://reviews.llvm.org/D41349 landing.

[shiva0217]

Remove parsing logic and get MCSubtargetInfo directly due to https://reviews.llvm.org/D41349 has been landing.

[asb]

Thanks for the update Shiva - a few minor comments in-line

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

94–100 ↗

(On Diff #128496)

This has slightly more nesting than normal LLVM style prefers. It could may be rewritten as:

// Nops in the base RISC-V ISA must be 4 bytes, but the compressed extension 
// introduces 2 byte nops.
if (!HasStdExtC && (Count % 4) != 0)
  return false;
if (HasStdExtC && (Count % 2) != 0)
  return false;

Probably better would be to change the function entry to this:

bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
unsigned MinNopLen = HasStdExtC ? 2 : 4;

if ((Count % MinNopLen) != 0)
  return false;

101 ↗

(On Diff #128496)

Nit: end comment with full stop.

104 ↗

(On Diff #128496)

Should have two spaces of indentation rather than four.

106 ↗

(On Diff #128496)

Nit: end comment with full stop.

[shiva0217]

Update the patch to reflect the comments.

[asb]

Thanks! Looks good to me.

llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
 namespace {
 class RISCVAsmBackend : public MCAsmBackend {
+  const MCSubtargetInfo &STI;
   uint8_t OSABI;
   bool Is64Bit;
 
 public:
-  RISCVAsmBackend(uint8_t OSABI, bool Is64Bit)
-      : MCAsmBackend(), OSABI(OSABI), Is64Bit(Is64Bit) {}
+  RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit)
+      : MCAsmBackend(), STI(STI), OSABI(OSABI), Is64Bit(Is64Bit) {}
   ~RISCVAsmBackend() override {}
 
   void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved) const override;
 
   std::unique_ptr<MCObjectWriter>
   createObjectWriter(raw_pwrite_stream &OS) const override;
void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
 
     report_fatal_error("RISCVAsmBackend::relaxInstruction() unimplemented");
   }
 
   bool writeNopData(uint64_t Count, MCObjectWriter *OW) const override;
 };
 
 bool RISCVAsmBackend::writeNopData(uint64_t Count, MCObjectWriter *OW) const {
-  // Once support for the compressed instruction set is added, we will be able
-  // to conditionally support 16-bit NOPs
-  if ((Count % 4) != 0)
+  bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
+  unsigned MinNopLen = HasStdExtC ? 2 : 4;
+
+  if ((Count % MinNopLen) != 0)
     return false;
 
-  // The canonical nop on RISC-V is addi x0, x0, 0
-  for (uint64_t i = 0; i < Count; i += 4)
+  // The canonical nop on RISC-V is addi x0, x0, 0.
+  uint64_t Nop32Count = Count / 4;
+  for (uint64_t i = Nop32Count; i != 0; --i)
     OW->write32(0x13);
 
+  // The canonical nop on RVC is c.nop.
+  if (HasStdExtC) {
+    uint64_t Nop16Count = (Count - Nop32Count * 4) / 2;
+    for (uint64_t i = Nop16Count; i != 0; --i)
+      OW->write16(0x01);
+  }
+
   return true;
 }
 
 static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
                                  MCContext &Ctx) {
   unsigned Kind = Fixup.getKind();
   switch (Kind) {
   default:

 } // end anonymous namespace
 
 MCAsmBackend *llvm::createRISCVAsmBackend(const Target &T,
                                           const MCSubtargetInfo &STI,
                                           const MCRegisterInfo &MRI,
                                           const MCTargetOptions &Options) {
   const Triple &TT = STI.getTargetTriple();
   uint8_t OSABI = MCELFObjectTargetWriter::getOSABI(TT.getOS());
-  return new RISCVAsmBackend(OSABI, TT.isArch64Bit());
+  return new RISCVAsmBackend(STI, OSABI, TT.isArch64Bit());
 }

llvm/trunk/test/MC/RISCV/cnop.s

+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
+# RUN:     | llvm-objdump -mattr=+c -d - | FileCheck -check-prefix=CHECK-INST %s
+
+# alpha and main are 8 byte alignment
+# but the alpha function's size is 6
+# So assembler will insert a c.nop to make sure 8 byte alignment.
+
+        .text
+       .p2align        3
+       .type   alpha,@function
+alpha:
+# BB#0:
+       addi    sp, sp, -16
+       c.lw    a0, 0(a0)
+# CHECK-INST: c.nop
+.Lfunc_end0:
+       .size   alpha, .Lfunc_end0-alpha
+                                        # -- End function
+       .globl  main
+       .p2align        3
+       .type   main,@function
+main:                                   # @main
+# BB#0:
+.Lfunc_end1:
+       .size   main, .Lfunc_end1-main
+                                        # -- End function