[PATCH] D43515: More math intrinsics for conservative math handling

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added inline comments.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:528
 
+    STRICT_FP_TO_SINT,
+    STRICT_FP_TO_UINT,
----------------
kpn wrote:
> craig.topper wrote:
> > These need comments. Does STRICT_FP_ROUND have the TRUNC argument that FP_ROUND has? 
> I could rearrange and lump them in with the non-strict versions of each. Then I'd just need one extra line restating that the STRICT_ versions prevent optimizations.
> 
> Yes, STRICT_FP_ROUND does have the TRUNC argument that FP_ROUND has, but it is currently always zero. Fixing this require rerouting this one strict node to go through the same codepath as the non-strict node. That would make it different from all the other constrained nodes. Should I go ahead and make that change? I _think_ it is safe if the TRUNC argument really does work like it is documented.
> 
> I also just noticed that I need to put back STRICT_FP_TO_UINT in at least one place. It should be everywhere STRICT_FP_TO_SINT is handled _except_ in the default lowering.
This ordering does not mesh with what is already in place. The STRICT_XXX opcodes are currently clustered together, where these new opcodes are grouped with their non-strict counterparts.

I'm not opposed to grouping the corresponding non-strict and strict opcodes, but that would probably be better left to a separate patch. I think it makes sense to keep everything uniform until a final decision is made, for clarity's sake.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2808
     break;
   case ISD::SIGN_EXTEND_INREG: {
     EVT ExtraVT = cast<VTSDNode>(Node->getOperand(1))->getVT();
----------------
This doesn't seem correct. Shouldn't Node->getOperand(0) be the argument for FP_ROUND and the Chain for STRICT_FP_ROUND?  Same for FP_EXTEND too. 

Also, does using a truncating store provide us with the same trapping behavior as an explicit trunc instruction? I don't know off the top of my head, but it may be different. To be fair, a user probably won't care too much about optimizing away a trap, but the purists might.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2868
       Results.push_back(Tmp1);
     break;
+  case ISD::STRICT_FP_TO_UINT:
----------------
This seems incorrect, unless I missed something. The first operand to FP_TO_SINT should be the argument, but the first operand to STRICT_FP_TO_SINT should be the Chain. It does not look like  expandFP_TO_SINT accounts for that.


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

https://reviews.llvm.org/D43515