[PATCH] D29934: [RISCV 12/n] Codegen support for memory operations

[Ondrej Glasnak via Phabricator via llvm-commits]

glasnak added inline comments.


================
Comment at: lib/Target/RISCV/RISCV.h:21
+#include "llvm/CodeGen/MachineOperand.h"
+#include "llvm/MC/MCInst.h"
 #include "llvm/Target/TargetMachine.h"
----------------
this include is not really needed when you have "class MCInst;" below.
Same for including MachineOperand.h, wouldn't "class MachineOperand;" be sufficient?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:77
+SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
+                                                SelectionDAG &DAG) const {
+  SDLoc DL(Op);
----------------
I'm honestly confused. Why is LowerOperation CamelCase, but lowerGlobalAddress is camelCase? I've seen this inconsistency of function naming elsewhere too, and all I can think of is that LLVM is changing to camelCase function code style just very very slowly. Is this correct? 

Anyway, I'll leave the decision up to you, but these two functions have almost identical signature and a different style, that doesn't seem right.


================
Comment at: lib/Target/RISCV/RISCVMCInstLower.cpp:25
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 
----------------
This file now seems to be working perfectly fine with just these includes:
#include "RISCV.h"
#include "MCTargetDesc/RISCVMCExpr.h"
#include "llvm/CodeGen/AsmPrinter.h"
#include "llvm/MC/MCAsmInfo.h"
#include "llvm/MC/MCContext.h"
please remove the others.


https://reviews.llvm.org/D29934