[PATCH] D29933: [RISCV 11/n] Initial codegen support for ALU operations

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM so as to unblock patches.  I don't see anything obviously wrong with these patches though I might be missing details as this is an area I'm not hugely familiar with.

As a side note, the amount of boilerplate needed here is disturbing.  I'd have expected us to be able to avoid that through tablegen, or well structured class structures or something.



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:95
+  for (auto &VA : ArgLocs) {
+    if (VA.isRegLoc()) {
+      // Arguments passed in registers.
----------------
minor: use early return for the non-reg case


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:109
+      default:
+        DEBUG(dbgs() << "LowerFormalArguments Unhandled argument type: "
+                     << RegVT.getEVTString() << "\n");
----------------
minor: please put the default at the top to make it obvious.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:143
+  // Copy the result values into the output registers.
+  for (unsigned i = 0, e = RVLocs.size(); i < e; ++i) {
+    CCValAssign &VA = RVLocs[i];
----------------
for-each loop?


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:191
 
-let rs1=0, rd=0 in {
+let rs1 = 0, rd = 0 in {
   def ECALL  : FI<0b000, 0b1110011, (outs), (ins), "ecall", []> {
----------------
Please, please, please do not post whitespace diffs.  Feel free to commit these without review.


================
Comment at: lib/Target/RISCV/RISCVMCInstLower.cpp:31
+
+  for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
+    const MachineOperand &MO = MI->getOperand(i);
----------------
for-each


https://reviews.llvm.org/D29933