This is an archive of the discontinued LLVM Phabricator instance.

[InstructionSimplify] handle denormal constant input for fcmp
ClosedPublic

Authored by shchenz on Jun 27 2022, 7:14 AM.

Details

Summary

Handle denormal input for fcmp instructions according to denormal float handling mode.

Diff Detail

Event Timeline

shchenz created this revision.Jun 27 2022, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 7:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
shchenz requested review of this revision.Jun 27 2022, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 7:14 AM

Thanks, I'd started looking at how to include fcmp but you were quicker.

llvm/lib/Analysis/ConstantFolding.cpp
1345–1347

If the denormal mode is used as an argument instead of a function pointer to obtain it, then DenormMode.Input or DenormMode.Output can be used directly instead of the mode struct plus a selector.

llvm/lib/Analysis/InstructionSimplify.cpp
3896–3897

The SimplifyQuery Q has a pointer to the instruction, so the denormal mode can be obtained via its parent rather than needing the denormal mode as an additional argument.

3903–3904

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.

shchenz updated this revision to Diff 440858.Jun 28 2022, 10:42 PM

address @dcandler comments

shchenz marked an inline comment as done.Jun 28 2022, 10:47 PM

Thanks for review @dcandler . Updated accordingly.

llvm/lib/Analysis/ConstantFolding.cpp
1345–1347

Good point. Done.

llvm/lib/Analysis/InstructionSimplify.cpp
3896–3897

I chose not to use SimplifyQuery Q before is because the instruction in Q may be nullptr. Seems using the instruction inside Q also does not impact the motivated case, so I use it now.

3903–3904

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.

spatel added inline comments.Jun 29 2022, 7:27 AM
llvm/lib/Analysis/ConstantFolding.cpp
1386

Prefer dyn_cast rather than isa+cast. So it could be written like this:

// Flush denormal inputs if needed.
if (auto *LCFP = dyn_cast<ConstantFP>(LHS)) {
  const fltSemantics &FltSema = LCFP->getType()->getFltSemantics();
  LHS = FlushFPConstant(LCFP, F->getDenormalMode(FltSema).Input);
}
if (auto *RCFP = dyn_cast<ConstantFP>(RHS)) {
  const fltSemantics &FltSema = RCFP->getType()->getFltSemantics();
  RHS = FlushFPConstant(RCFP, F->getDenormalMode(FltSema).Input);
}

// Calculate constant result.
Constant *C = ConstantFoldBinaryOpOperands(Opcode, LHS, RHS, DL);

// Flush denormal output if needed.
if (auto *CFP = dyn_cast<ConstantFP>(C)) {
  const fltSemantics &FltSema = CFP->getType()->getFltSemantics();
  C = FlushFPConstant(CFP, F->getDenormalMode(FltSema).Output);
}

return C;
llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll
762–763

Please add more tests to verify that we have the expected behavior for other denormal modes. These would be similar to what was added with D116952 (see tests in this file above here).

772–773

This is identical to the previous test?

dcandler added inline comments.Jun 29 2022, 11:42 AM
llvm/lib/Analysis/InstructionSimplify.cpp
3896–3897

In the previous review the view was that if there is no parent instruction/function available to get the denormal mode from, then it is okay to default to IEEE.

3903–3904

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.

llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll
1109–1111

Since compares only take floating point inputs, another relevant test would be to ensure compares don't get folded when only an output mode is set (attributes #1 and #2) but do get folded on an input mode (#3 and #4).

shchenz updated this revision to Diff 441263.Jun 29 2022, 10:18 PM
shchenz marked an inline comment as done.

address comments

shchenz marked 5 inline comments as done.Jun 29 2022, 10:19 PM

Thanks. @spatel @dcandler

Could you please help to have another look?

llvm/lib/Analysis/InstructionSimplify.cpp
3903–3904

Fair.

spatel added inline comments.Jun 30 2022, 4:21 AM
llvm/include/llvm/Analysis/ConstantFolding.h
69–77

Since we are updating this comment, please remove the function name at the start:
"Don’t duplicate function or class name at the beginning of the comment."
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Also, change the wording slightly:

For fcmp, denormal inputs may be flushed based on the denormal handling mode.
llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll
769

Here and in other tests - I didn't recognize the hex value as a denorm at first sight. It would be easier to see with the leading zeros shown explicitly.

772–773

Is this still the same as the previous test? It would be better to make the denorm constant as operand 0 and maybe vary the fcmp predicate for better coverage.

shchenz updated this revision to Diff 441371.Jun 30 2022, 6:04 AM
shchenz marked 4 inline comments as done.

address @spatel comments

  • add more test
  • comments format fix
llvm/include/llvm/Analysis/ConstantFolding.h
69–77

Thanks for remind.

llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll
772–773

Sorry, the attribute here should be #4.

spatel accepted this revision.Jun 30 2022, 6:30 AM

LGTM - but please wait a bit in case @dcandler has any other comments.

llvm/include/llvm/Analysis/ConstantFolding.h
73

denormal -> Denormal

This revision is now accepted and ready to land.Jun 30 2022, 6:30 AM
dcandler accepted this revision.Jun 30 2022, 8:00 AM

LGTM too, thanks.

shchenz updated this revision to Diff 441626.Jul 1 2022, 12:50 AM
shchenz marked an inline comment as done.

address comments and rebase

This revision was landed with ongoing or failed builds.Jul 1 2022, 1:07 AM
This revision was automatically updated to reflect the committed changes.