[PATCH] D48556: Fix asserts in AMDGCN fmed3 folding by handling more cases of NaN

[Alan Baker via Phabricator via llvm-commits]

alan-baker added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:3429-3432
+    // Checking for NaN before canonicalization provides better fidelity for
+    // mapping fclamp operations to fmed3. This will still not properly model
+    // NaN in fclamp if the intrinsic is canonicalized prior to the NaN being
+    // determined.
----------------
arsenm wrote:
> This comment is sort of confusing me. Is it saying there is still a problem? 
> 
> Probably shouldn't mention fclamp, as no operation called fclamp is involved here.
If fmed3(x,y,z) was used to model fclamp(x,y,z) (i.e. fmin(fmax(x,y), z)), and x or y are determined to be a constant prior to determining one of the operands is a NaN, then the canonicalization will swap operands around. If later an operand is determined to be a NaN then the evaluation of the fmed3 may no longer match the original fclamp operation. So there is a potential problem if not all the constants are determined prior to the invocation of inst combine, but are for subsequent invocations. This problem is specifically for modelling fclamp, which is why I mentioned it here. I can remove the comment if you prefer, but LLPC maps fclamp directly to fmed3 (and I'm not sure I see a performant alternative without introducing a fclamp intrinsic).


Repository:
  rL LLVM

https://reviews.llvm.org/D48556