[PATCH] D128647: [InstructionSimplify] handle denormal constant input for fcmp

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 5 inline comments as done.
shchenz added a comment.

Thanks. @spatel @dcandler

Could you please help to have another look?



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3909
+                               /* IsOutput */ false);
       return ConstantFoldCompareInstOperands(Pred, CLHS, CRHS, Q.DL, Q.TLI);
+    }
----------------
dcandler wrote:
> shchenz wrote:
> > dcandler wrote:
> > > I feel it would be better to handle the denormals inside ConstantFoldCompareInstOperands (it would only need the instruction pointer from Q) as that would correct the result for all compare folding, whereas this patch currently only corrects the call from InstSimplify.
> > Sure. I change to handle denormal input in `ConstantFoldCompareInstOperands` now. However `ConstantFoldCompareInstOperands` is not just for float compare, it is also for integer compare. So it also makes to me that we don't add a parameter specific for float in the function.
> `ConstantFoldCompareInstOperands` does do both float and integer compares, so my thinking was to only start worrying about denormals inside that function after we can establish whether it is dealing with integer or float instructions. That was why I suggested using an instruction pointer as an additional parameter: regardless of the type, a pointer to the instruction being folding should be available in most cases, and then this can be refined down to the denormal mode within the function only when needed.
> 
> This would make it easier to update calls to `ConstantFoldCompareInstOperands` elsewhere (e.g. `ConstantFold` in `SimplifyCFG.cpp`), since the existing instruction pointer can just be passed down, rather than having to determine a denormal mode for every usage, especially when a denormal mode may not be needed.
Fair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128647