[PATCH] D71742: Added intrinsics for access to FP environment

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:12381
+  SDValue Callee = getExternalSymbol(TLI->getLibcallName(LC),
+                                     TLI->getPointerTy(getDataLayout()));
+  TargetLowering::CallLoweringInfo CLI(*this);
----------------
arsenm wrote:
> If not getting it from the actual declaration, should probably use the default program address space (don't really care if you fix it here, 90% of the uses of this are wrong as it is)
IIRC default program address space is for function code. I put `getPointerMemTy` instead of `getPointerTy`, although now it behave in the same way. I don't know to put correct address space here, as declaration of the function is unavailable.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6551-6553
+    // Use GET_FPENV if it is legal or custom. Otherwise use memory-based node
+    // and temporary storage in stack.
+    if (TLI.isOperationLegalOrCustom(ISD::SET_FPENV, EnvVT)) {
----------------
arsenm wrote:
> Can this just go into an Expand action like normal? We randomly expand certain things in the DAG builder but the inconsistency is annoying 
This code does the same job as other cases in this function, - it creates relevant DAG nodes. Their expansion occurs in `SelectionDAG::Legalize`, as for other nodes. The only difference is using two kings of nodes, register and memory-based ones.

Using only one kind of node could simplify code in this function but complicates code in other parts. On some targets FP environment is long and corresponding integer type is illegal (i256 on x86). In such case DAG nodes have to be expanded early, when DAG legalizes types. If FP environment can be represented by a legal type, the expansion occurs late, in `SelectionDAG::Legalize`. There were two points when the expansion occurred. This approach was used in previous versions of this patch, it complicated the implementation and made it fragile.

Using separate nodes for operations that involves memory allows to treat all FP environment operations in uniform manner, at the cost of small complication of this function.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6591
+      Chain =
+          DAG.getStore(Chain, sdl, Env, Temp, MPI, DLayout.getStackAlignment(),
+                       MachineMemOperand::MOStore);
----------------
arsenm wrote:
> This should use the alignment for EnvVT, not the datalayout's stack alignment 
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71742