[PATCH] D109371: [RISCV][RFC] Add LLVM support for RISC-V overlay system

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:627
 
+  // For overlay functions, replace the default section name with ".ovlinput"
+  if (isa<Function>(GO) &&
----------------
Period at the end of the comment.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:633
+  // For overlay data, also change the section prefix to ".ovlinput", since
+  // the overlay system mixes the two
+  if (isa<GlobalVariable>(GO) &&
----------------
Period at the end of the comment.


================
Comment at: llvm/lib/LTO/LTO.cpp:1122
                                               : GlobalValue::UnnamedAddr::None);
-      if (EnableLTOInternalization && R.second.Partition == 0)
+      bool isOverlayCall = false;
+      if (isa<Function>(GV) &&
----------------
First letter of variable names should be capitalized.


================
Comment at: llvm/lib/LTO/LTO.cpp:1125
+          cast<Function>(GV)->hasFnAttribute("overlay-call"))
+        isOverlayCall = true;
+      if (EnableLTOInternalization && R.second.Partition == 0 && !isOverlayCall)
----------------
Can this be

```
bool isOverlayCall = isa<Function>(GV) && cast<Function>(GV)->hasFnAttribute("overlay-call");
```


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2247
+  default: return false;
+  case RISCV::X1: return STI.getFeatureBits()[RISCV::FeatureReserveX1];
+  case RISCV::X2: return STI.getFeatureBits()[RISCV::FeatureReserveX2];
----------------
These lines can be shortened slightly to

```
case RISCV::X1: return STI.hasFeature(RISCV::FeatureReserveX1);
```


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2285
+  for (unsigned i = 0; i < Desc.getNumDefs(); i++) {
+    if (Inst.getOperand(i).isReg() && isRegisterReserved(Inst.getOperand(i).getReg(), getSTI()) && WarnOnReservedReg)
+      Warning(Inst.getLoc(), "Instruction modifies reserved register");
----------------
80 columns


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:166
+  unsigned NumInstrs = 0;
+  if (Opcode == RISCV::PseudoOVLCALL ||
+      Opcode == RISCV::PseudoOVLCALLIndirect) {
----------------
Is this outer if necessary? The inner if and the assert in the inner else should be enough to check the two opcodes.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:176
+      // Extract the call symbol that should be targetted by this call.
+      assert(MI.getOperand(0).isExpr() && "Something has gone wrong?");
+      const RISCVMCExpr *RCallExpr = cast<RISCVMCExpr>(MI.getOperand(0).getExpr());
----------------
This assert isn't really needed. The getExpr() call on the next line will assert.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:178
+      const RISCVMCExpr *RCallExpr = cast<RISCVMCExpr>(MI.getOperand(0).getExpr());
+      assert(RCallExpr->getSubExpr()->getKind() == MCExpr::SymbolRef);
+      const MCSymbolRefExpr *Sym = cast<MCSymbolRefExpr>(RCallExpr->getSubExpr());
----------------
This assert isn't really needed, the cast<MCSymbolRefExpr> on the next line will assert if it can't cast.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:180
+      const MCSymbolRefExpr *Sym = cast<MCSymbolRefExpr>(RCallExpr->getSubExpr());
+      bool isOVL = RCallExpr->getKind() == RISCVMCExpr::VK_RISCV_OVLCALL;
+      if (!isOVL && (RCallExpr->getKind() != RISCVMCExpr::VK_RISCV_OVL2RESCALL))
----------------
Capitalize isOVL


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2887
+  }
+  else if (auto *GVar = dyn_cast<GlobalVariable>(GV)) {
+    if (GVar->hasAttribute("overlay-data")) {
----------------
else should be on the same line as the closing curly brace above


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2892
+      LoFlags = RISCVII::MO_OVL_LO;
+      LoFlags = RISCVII::MO_OVL_HI;
+    }
----------------
Should this be HiFlags? Why wasn't this caught by the assert that both flags are non-zero in RISCVTargetLowering::getAddr


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8352
   // split it and then direct call can be matched by PseudoCALL.
+  bool isOVLCC = false;
+  bool isIndirect = false;
----------------
Capitalize


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8367
+    }
+    else if (!getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV))
       OpFlags = RISCVII::MO_PLT;
----------------
Move after close curly brace on previous line


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8379
+    }
+    else if (!getTargetMachine().shouldAssumeDSOLocal(*MF.getFunction().getParent(),
                                                  nullptr))
----------------
Same


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8423
+  if (isIndirect &&
+      MF.getFunction().hasFnAttribute("overlay-call")) {
+    Chain = DAG.getNode(RISCVISD::OVLCALL_INDIRECT, DL, NodeTys, Ops);
----------------
Does this condition fit on in 80 columns on one line? It looks like it should


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8424
+      MF.getFunction().hasFnAttribute("overlay-call")) {
+    Chain = DAG.getNode(RISCVISD::OVLCALL_INDIRECT, DL, NodeTys, Ops);
+  } else if (isOVLCC) {
----------------
Looks like you could get have one getNode call after the ifs and make the ifs choose only the opcode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109371