[PATCH] D139481: NVPTX: Cleanup check for denormal mode

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.cpp:374
+        Action.IsHalfTy ? APFloat::IEEEhalf() : APFloat::IEEEsingle());
+    bool FtzEnabled = Mode.Output == DenormalMode::PreserveSign;
 
----------------
tra wrote:
> arsenm wrote:
> > tra wrote:
> > > arsenm wrote:
> > > > Looks broken for double to me but I don't know what the options are
> > > 
> > > Indeed. I do not understand why getDenormalMode appears to single out fp32 as a special case.
> > > 
> > > 
> > Because turn on f32 flush is the option the languages actually expose. denormal-fp-math-f32 acts as an override to denormal-fp-math
> OK, then the code is (and has been) broken for 'double'. I do not think we want to override denormals mode for doubles using `denormal-fp-math-f32`.
> On the other hand, it's no more broken than it was before, so we can probably live with that until we figure out how to fix this.
> 
> https://llvm.org/docs/LangRef.html says:
> >" denormal-fp-math-f32" [...] Not all targets support separately setting the denormal mode per type, and no attempt is made to diagnose unsupported uses. Currently this attribute is respected by the AMDGPU and NVPTX backends.
> 
> On a side note, I wonder if the denormals handling mode may need to grow a similar override for fp16, which also has .ftz instruction variants.
But do those imply a change in the default FP mode, or are they just operations that have flushing semantics in the default mode


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

https://reviews.llvm.org/D139481