[PATCH] D71338: [VE,#1] Scalar code generation

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp:113-123
+static const MCPhysReg IntRegs[64] = {
+    VE::SX0,  VE::SX1,  VE::SX2,  VE::SX3,  VE::SX4,  VE::SX5,  VE::SX6,
+    VE::SX7,  VE::SX8,  VE::SX9,  VE::SX10, VE::SX11, VE::SX12, VE::SX13,
+    VE::SX14, VE::SX15, VE::SX16, VE::SX17, VE::SX18, VE::SX19, VE::SX20,
+    VE::SX21, VE::SX22, VE::SX23, VE::SX24, VE::SX25, VE::SX26, VE::SX27,
+    VE::SX28, VE::SX29, VE::SX30, VE::SX31, VE::SX32, VE::SX33, VE::SX34,
+    VE::SX35, VE::SX36, VE::SX37, VE::SX38, VE::SX39, VE::SX40, VE::SX41,
----------------
I assume these are all in one class, so you could just directly access the MCRegisterClass?


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:95
+          Base = CurDAG->getTargetFrameIndex(
+              FIN->getIndex(), TLI->getPointerTy(CurDAG->getDataLayout()));
+        } else {
----------------
Can't this just re-use the existing type instead of calling TLI->getPointerTy?


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:118
+
+  if (Addr.getOpcode() == ISD::ADD) {
+    if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1)))
----------------
A future improvement would be to use isBaseWithConstantOffset here to catch the annoying add->or case


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:136-139
+// Note: This function was copied from, and is essentially identical
+// to ARMISelDAGToDAG::SelectInlineAsm. It is very unfortunate that
+// such hacking-up is necessary; a rethink of how inline asm operands
+// are handled may be in order to make doing this more sane.
----------------
Why is this necessary exactly? I didn't know any targets had to manually process inline asm selection


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:305-307
+  PointerType *Ty = cast<PointerType>(CalleeFn->arg_begin()->getType());
+  Type *ElementTy = Ty->getElementType();
+  return DAG.getDataLayout().getTypeAllocSize(ElementTy);
----------------
Relying on pointer element type. I think we already have a correct opaque pointer way to do this for sret?


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:738
+
+  // FIXME: VE's I128 stuff is not investivated yet
+  if (!1) {
----------------
New word 'investivated'


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:1040
+      return DAG.getNode(ISD::ADD, DL, VT, GlobalBase, HiLo);
+    } else {
+      // Create following instructions for not local linkage PIC code.
----------------
No else after return


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:1289
+
+  EVT PtrVT = TLI.getPointerTy(MF.getDataLayout());
+
----------------
Just reuse the existing type?


================
Comment at: llvm/lib/Target/VE/VEISelLowering.cpp:1916
+
+    unsigned TReg = MRI->createVirtualRegister(&VE::I64RegClass);
+    unsigned Tmp1 = MRI->createVirtualRegister(&VE::I64RegClass);
----------------
s/unsigned/Register, here and many other places


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.cpp:53
+      MI.getOpcode() == VE::LDQri    // F128 (pseudo)
+  ) {
+    if (MI.getOperand(1).isFI() && MI.getOperand(2).isImm() &&
----------------
Previous line


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.cpp:74
+      MI.getOpcode() == VE::STQri    // F128 (pseudo)
+  ) {
+    if (MI.getOperand(0).isFI() && MI.getOperand(1).isImm() &&
----------------
Previous line


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.cpp:332
+
+  // report_fatal_error("removeBranch is not implemented yet");
+}
----------------
Dead code


================
Comment at: llvm/lib/Target/VE/VERegisterInfo.cpp:200-203
+  if (1) {
+    LLVM_DEBUG(dbgs() << "replaceFI: "; MI.dump());
+  }
+
----------------
Leftover debugging junk


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71338