[PATCH] D130141: [RISCV] Add part support of RISCV Zc Extension (Zca, Zcb, Zcmp)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Where is it documented that these extensions are ratified?

Putting 3 extensions in one patch is a guaranteed way to make it take longer to approved. The smaller you can make patches the quicker individual pieces can be reviewed and approved. I would also recommend putting new optimization passes in separate patches. MC layer and CodeGen should be separate patches.

Please run clang-format on patches.



================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:305
 
+  bool hasZca = false; 
   Optional<StringRef> Attr = Attributes.getAttributeString(RISCVAttrs::ARCH);
----------------
Capitalize variable names


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:343
 
+  if(hasZca){
+    Features.AddFeature("c",false);
----------------
Space after `if`. Space before curly brace


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:344
+  if(hasZca){
+    Features.AddFeature("c",false);
+    Features.AddFeature("experimental-zca");
----------------
Space before `false`


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:347
+  }
+    
+
----------------
Extra blank line


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2109
+
+  StringRef Memonic =
+      static_cast<RISCVOperand *>(Operands.front().get())->getToken();
----------------
Memonic -> Mnemonic


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:194
+  unsigned opcode = MI->getOpcode();
+  bool isRV64 = STI.getFeatureBits()[RISCV::Feature64Bit];
+  int64_t spimm = 0;
----------------
Capitalize variable names


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:416
+  auto Imm = MO.getImm();
+  if(Imm <4)
+    assert(0 && "EABI is currently not implemented");
----------------
Space after '<'


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:1
+//===- RISCVMoveOptimizer.cpp - RISCV move opt. pass -------===//
+//
----------------
Header banner should be exactly 80 characters long


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:20
+
+#define RISCV_MOVE_OPT_NAME "RISC-V Zce move merging pass"
+
----------------
This says Zce but Zce is not mentioned in your patch description.


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:225
+
+  Subtarget = &static_cast<const RISCVSubtarget &>(Fn.getSubtarget());
+  if (!Subtarget->hasStdExtZcmp()) {
----------------
Use the template version of getSubtarget that does this cast.


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:230
+
+  TII = static_cast<const RISCVInstrInfo *>(Subtarget->getInstrInfo());
+  TRI = Subtarget->getRegisterInfo();
----------------
getInstrInfo() on RISCVSubtarget* returns RISCVInstrInfo * this case shouldn't be needed.


================
Comment at: llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp:1
+//===- RISCVOptimizePushPop.cpp - RISCV Push/Pop opt. pass -------===//
+//
----------------
80 columns


================
Comment at: llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp:23
+
+
+
----------------
too many blank lines


================
Comment at: llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp:27
+  struct RISCVPushPopOpt: public MachineFunctionPass{
+      static char ID;
+
----------------
This is overly indented


================
Comment at: llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp:44
+
+      std::map<MachineInstr *, int> retValMap;
+
----------------
Capitalize variable names.

Can this a be a DenseMap? std::map is an ordered map which doesn't make much sense for pointers.


================
Comment at: llvm/lib/Target/RISCV/RISCVOptimizePushPop.cpp:73
+    DebugLoc DL = NextI->getDebugLoc();
+    auto retValInfo = retValMap.find(&(*MBBI));
+    if (retValInfo == retValMap.end())
----------------
Capitalize variable names


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130141/new/

https://reviews.llvm.org/D130141