[PATCH] D150011: AMDGPU: Pattern match fract instructions in AMDGPUCodeGenPrepare

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

I just have some minor nits. I can't comment on the correctness of the logic so I will leave it up to someone with more experience w.r.t FP ops to approve

> We previously had some wrong patterns for this under unsafe math which I've removed.

Would it be possible to split that in a separate patch? Without that it's a bit difficult to tell if a test changed because of the combine, or the pattern.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:228
+  Value *matchFractPat(IntrinsicInst &I);
+  Value *applyFractPat(IRBuilder<> &Builder, Value *FractArg);
+
----------------
nit: isn't `IRBuilderBase` better?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1424
+  // Match fract pattern with nan check.
+  if (match(Cond, m_FCmp(Pred, m_Value(CmpVal), m_NonNaN()))) {
+    IRBuilder<> Builder(&I);
----------------
(optional) small nit: maybe use `!match` and an early return to reduce nesting? Could also do `!Fract` and early return. 




================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp:1682
+  Type *Ty = FractArg->getType()->getScalarType();
+  for (int I = 0, E = FractVals.size(); I != E; ++I) {
+    ResultVals[I] =
----------------
nit: use `unsigned` as it's a size type?


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

https://reviews.llvm.org/D150011