[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

Wed May 1 13:16:30 PDT 2019

andrew.w.kaylor added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2787
+  case ISD::STRICT_FP_ROUND:
+    Tmp1 = EmitStackConvert(Node->getOperand(1), 
+                            Node->getValueType(0),
----------------
cameron.mcinally wrote:
> andrew.w.kaylor wrote:
> > kpn wrote:
> > > cameron.mcinally wrote:
> > > > andrew.w.kaylor wrote:
> > > > > kpn wrote:
> > > > > > andrew.w.kaylor wrote:
> > > > > > > Can this cause the wrong rounding mode to be used? For X86 targets I would guess it will result in an instruction that uses the runtime rounding mode, but I'm not sure we can count on that for all targets. Also, if the value being converted is a constant, might this get folded using the default rounding mode? And if it is a constant and we knew the rounding mode based on an argument to the intrinsic, we might want to fold it (though ideally that would have happened before this).
> > > > > > Isn't this an argument for a strict load node types and maybe a strict store type as well? 
> > > > > > 
> > > > > > I'd be surprised if we were doing constant folding of a store+load combination, but if we are then either a strict load or store node would disable that folding.
> > > > > > 
> > > > > > Strictly speaking, using the runtime rounding mode would be incorrect if that wasn't what was specified in the rounding mode field of the intrinsic. So even X86 could be wrong.
> > > > > > 
> > > > > > We aren't doing anything with the rounding and exception arguments yet for any of the new intrinsics. So how about I file a bug noting that fact and noting that we need to not forget about this case? Then we can move on and come back later.
> > > > > Actually, if the runtime rounding mode doesn't match the rounding argument for non-dynamic cases that's a user error or a bug somewhere upstream in the compiler. The rounding mode argument is supposed to tell us what the rounding mode is at this point in the program. It is not supposed to control the rounding mode. The non-dynamic rounding modes are essentially an "assume" kind of directive.
> > > > > 
> > > > > I see that the constant folding case I'm concerned about will actually be covered by one of your test cases, so I think we can safely move forward with this the way you have it.
> > > > > Can this cause the wrong rounding mode to be used? For X86 targets I would guess it will result in an instruction that uses the runtime rounding mode, but I'm not sure we can count on that for all targets.
> > > > 
> > > > I think it's more severe than that. The details are not fresh in my mind, but IIRC EmitStackConvert(...) emits a straight truncate on X86 (well, at least for one case I looked at. Could be wrong too.). We probably shouldn't be calling it for a strict round, and rather go to an instruction that honors rounding mode.
> > > My impression is that EmitStackConvert() is one of those functions that is only used as a last resort when an ISA lacks a good reg+reg instruction. So this is probably already handled for most cases.
> > > 
> > > If you know of a way to write a strict FP test case that uses it I'd love to see it. And if you have one then we should probably turn it into a library call to honor the rounding mode.
> > @cameron.mcinally , what do you mean by "a straight truncate"?
> Nope, I made a mistake. EmitStackConvert(...) ends up creating a truncstore, which could end up as an FST. I had mistakenly thought that this rounded to nearest, but it looks like it uses the FPU control word.
> 
> Assuming that fenv.h sets the FPU control word correctly (I didn't check), it should be fine. 
Regarding fenv.h, if you use the fesetround() function from fenv.h to set the rounding mode, it will update both MXCSR and FPCW. If you use an intrinsic or inline assembly or whatever to set one of them without the other, you're on your own (and fegetround() will fail if they have different settings).

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

https://reviews.llvm.org/D55897