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

Fri Jul 26 09:19:25 PDT 2019

kpn added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2790
+        == TargetLowering::Legal)
+      break;
     Tmp1 = EmitStackConvert(Node->getOperand(1), 
----------------
uweigand wrote:
> kpn wrote:
> > 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.
> > Is it documented anywhere that the truncating store will not raise exceptions?
> 
> Common code **assumes** that the truncating store will not raise FP exceptions, just like it assumes **any** DAG operation, except for the STRICT_... ones, will not raise FP exceptions.  (Now, since it is a store, common code assumes that it may raise an exception because of memory access faults, but then again, in this specific use case, common code will recognize that the store targets a stack slot which can never fault.)  Therefore, common code might e.g. speculate the truncating store outside of a condition -- which would be incorrect if indeed FP exceptions are turned on.  Similarly, common code might schedule this truncating store across a function call that might change rounding modes.
> 
> I guess a way to correctly implement this transformation would be to expand a STRICT_FP_ROUND into a "strict" version of a truncating store (which currently does not exist).  But I don't think it would be worthwhile to add this until and unless this is actually useful on some target.
> 
> > It sounds like we're changing what Expand means for strict operations. 
> 
> Well, I guess that's true in the sense that for now, Expand for strict operations allows for two implementations:
> - either, implement the precise semantics of the operation in terms of other operations (this is the "traditional" meaning of Expand, and is in some cases doable for strict operations too, but usually only if you can implement a strict operations in terms of other **strict** operations);
> - or, fall back to the non-strict semantics if strict semantics are not possible because the target doesn't (yet) implement any of those at all
> 
> At some point in the future, we want the second option to go away, but we're not there yet.  But on targets that **do** implement strict semantics in the backend, common code should never use that second option.
> 
> > And there's plenty of code in place to do those expansions in ways that preserve the strictness. 
> 
> Not really?  All the existing expansions end up in solely non-strict DAG opcodes, so how could they preserve the strictness?
> 
> > 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.
> 
> No, long term the meaning should be the same again.  The fallback is the short-term thing.
> 
> Common code assumes that the truncating store will not raise FP exceptions, just like it assumes any DAG operation, except for the STRICT_... ones, will not raise FP exceptions. (Now, since it is a store, common code assumes that it may raise an exception because of memory access faults, but then again, in this specific use case, common code will recognize that the store targets a stack slot which can never fault.) Therefore, common code might e.g. speculate the truncating store outside of a condition -- which would be incorrect if indeed FP exceptions are turned on. Similarly, common code might schedule this truncating store across a function call that might change rounding modes.

In the STRICT EmitStackConvert() case the store+load are chained together _and_ that chain is spliced into the chain where the STRICT node was formerly located. Does common code do transformations to the SDAG that reorder the chain? I thought the point of the chain was to enforce ordering.

> I guess a way to correctly implement this transformation would be to expand a STRICT_FP_ROUND into a "strict" version of a truncating store (which currently does not exist). But I don't think it would be worthwhile to add this until and unless this is actually useful on some target.

Agreed.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65226