[PATCH] D114950: [RISCV] Promote large integers to constant pool

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:161
 
+static SDNode *selectGlobalImm(SelectionDAG *CurDAG, const SDLoc &DL,
+                               const MVT &VT, ConstantSDNode *ConstNode,
----------------
This makes it sound like the immediate is global which doesn't make sense. `selectImmWithConstantPool` or `selectImmUsingConstantPool` would be a better name, I think.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:162
+static SDNode *selectGlobalImm(SelectionDAG *CurDAG, const SDLoc &DL,
+                               const MVT &VT, ConstantSDNode *ConstNode,
+                               const RISCVSubtarget &Subtarget) {
----------------
MVT should always be passed by value. It's an 8-bit integer.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:168
+  SDValue Offset = CurDAG->getConstant(0, DL, VT);
+  // Since there is no data race, the chain could be the entry node.
+  return CurDAG->getMachineNode(RISCV::LD, DL, VT, Addr, Offset,
----------------
could -> can


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:169
+  // Since there is no data race, the chain could be the entry node.
+  return CurDAG->getMachineNode(RISCV::LD, DL, VT, Addr, Offset,
+                                CurDAG->getEntryNode());
----------------
LD assumes VT is MVT::i64. Should we assert that?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:520
+    if (!ImmNode)
+      ImmNode = selectGlobalImm(CurDAG, DL, VT, ConstNode, *Subtarget);
+
----------------
Can we call this from inside selectImm instead of returning nullptr? selectImm only has 1 caller right now, but it has add other callers in the past.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:114
+unsigned RISCVSubtarget::getMaxBuildIntCost() const {
+  // LoadLatency + 1
+  return 4;
----------------
LoadLatency is going to very for different CPUs so saying LoadLatency+1 and then having a hardcoded constant doesn't make sense.

I'm not opposed to the hard coded constant, it matches what gcc uses. I just don't like the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114950