[PATCH] D65226: [Strict FP] Allow custom operation actions

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added a comment.

So the PowerPC code regressions will be fixed once the strict tickets make it into the tree it sounds like.



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2790
+        == TargetLowering::Legal)
+      break;
     Tmp1 = EmitStackConvert(Node->getOperand(1), 
----------------
uweigand wrote:
> kpn wrote:
> > In what way does it not honor the strict properties?
> > 
> > Also, wouldn't it be a bug if we were asked to expand a strict node when the non-strict is legal?
> > In what way does it not honor the strict properties?
> 
> Well, the expansion is done by using a truncating store followed by a load.  The truncating store is a non-strict operation which will not raise exceptions.
> 
> > Also, wouldn't it be a bug if we were asked to expand a strict node when the non-strict is legal?
> 
> Why would that be a bug?  That's the current default behavior on all targets that don't (yet) support strict operations.   (All strict operations default to expand, which is taken to mean replace by the non-strict version.)
> 
>> In what way does it not honor the strict properties?
> 
> Well, the expansion is done by using a truncating store followed by a load. The truncating store is a non-strict operation which will not raise exceptions.

Is it documented anywhere that the truncating store will not raise exceptions? It seems weird to me since for IEEE targets a truncating store must change the format of the bits anyway. What if an exponent is out of range of the smaller type, for example?

How many non-IEEE targets are there? I'm well aware that hardware is still very current that supports S/370's radix 16 FP, plus the radix 10 FP, but are there any other current forms of not-at-all-IEEE floating point?

A different angle: if a target cannot use EmitStackConvert(), then isn't it that target's responsibility to make sure this code path is never used? In that case we wouldn't need this subtle code here.

Do we even have any tests for EmitStackConvert()? I remember having a very hard time finding one.

>> Also, wouldn't it be a bug if we were asked to expand a strict node when the non-strict is legal?
> 
> Why would that be a bug? That's the current default behavior on all targets that don't (yet) support strict operations. (All strict operations default to expand, which is taken to mean replace by the non-strict version.)

It sounds like we're changing what Expand means for strict operations. Previously it meant the same thing as it does for non-strict operations: use the fallback/default expansion. And there's plenty of code in place to do those expansions in ways that preserve the strictness. The strict and non-strict nodes have followed the same paths for the most part with the exception of strict { Custom, Promote } -> Expand.

Long term, on targets that support strict floating point, do we want Expand to have different meanings for strict and non-strict nodes? It worries me if they're different.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3707
+      return true;
+  }
+
----------------
Say, we can't emit libcalls for any old random thing. If we simply return false here then it will try -- and fail -- to emit a libcall. And failing to emit a libcall is _not_ fatal I'm pretty sure. So no tricky returning true when we didn't actually expand anything is needed here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65226