[PATCH] D130141: [RISCV] Add part support of RISCV Zc Extension (Zca, Zcb, Zcmp)
Craig Topper via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 19 21:19:58 PDT 2022
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
More information about the llvm-commits
mailing list