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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM with the getPointerMemTy thing reverted



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:12381
+  SDValue Callee = getExternalSymbol(TLI->getLibcallName(LC),
+                                     TLI->getPointerTy(getDataLayout()));
+  TargetLowering::CallLoweringInfo CLI(*this);
----------------
sepavloff wrote:
> 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.
pointer mem ty makes less sense 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6561
+    } else {
+      SDValue Temp = DAG.CreateStackTemporary(EnvVT, TempAlign.value());
+      int SPFI = cast<FrameIndexSDNode>(Temp.getNode())->getIndex();
----------------
CreateStackTemporary should be fixed to directly take Align input 


================
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)) {
----------------
sepavloff wrote:
> 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.
This hack doesn't require repeating in globalisel 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71742