[PATCH] D133340: [PowerPC][GISel]select floating point constant from TOC

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:664
+      MachinePointerInfo::getGOT(*MF), MachineMemOperand::MOLoad,
+      MRI.getType(DstReg), Align(MRI.getType(DstReg).getSizeInBits() / 8));
+
----------------
Is this really the alignment we want? Seems like we should use whatever the ABI alignment is for the type. 


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:672
+             .addConstantPoolIndex(CPI)
+             .addReg(PPC::X2)
+             .addMemOperand(MMO);
----------------
I understand that this is always `PPC::X2` for ELFv2, but can we use `getTOCPointerRegister()` instead?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCLegalizerInfo.cpp:53
 
+  getActionDefinitionsBuilder(G_FCONSTANT).lowerFor({S32, S64});
+  getActionDefinitionsBuilder(G_CONSTANT_POOL).legalFor({P0});
----------------
In SDAG, we have the capability to mark specific FP constants legal. Does a similar capability exist with GISel? Not using constant pool loads for constants such as `0.0` is quite important.
Perhaps a TODO is in order here?


================
Comment at: llvm/test/CodeGen/PowerPC/GlobalISel/fconstant-unsupported.ll:1
+; REQUIRES: asserts
+
----------------
arsenm wrote:
> This doesn't require asserts
Hmm, is it that producing FP constants using GISel is not supported or that GISel is not supported at all for BE and 32-bit? I imagine it is the latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133340