[PATCH] D41221: [RISCV] writeNopData support generate c.nop

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.
Herald added a subscriber: niosHD.

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



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:94-100
+  if (HasStdExtC) {
+    if ((Count % 2) != 0)
+      return false;
+  } else {
+    if ((Count % 4) != 0)
+      return false;
+  }
----------------
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;
```


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:101
+  }
   // The canonical nop on RISC-V is addi x0, x0, 0
+  uint64_t Nop32Count = Count / 4;
----------------
Nit: end comment with full stop.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:104
+  for (uint64_t i = Nop32Count; i != 0; --i)
+     OW->write32(0x13);
+ 
----------------
Should have two spaces of indentation rather than four.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:106
+ 
+  // The canonical nop on RVC is c.nop
+  if (HasStdExtC) {
----------------
Nit: end comment with full stop.


Repository:
  rL LLVM

https://reviews.llvm.org/D41221