This is an archive of the discontinued LLVM Phabricator instance.

Eliminate ftrunc if source is know to be rounded
ClosedPublic

Authored by rampitec on Sep 29 2017, 12:48 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Sep 29 2017, 12:48 PM
hfinkel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10705 ↗(On Diff #117204)

Is there any way we can generalize this? All of the operations listed here are like ftrunc in this regard (they're idempotent for integer inputs).

rampitec added inline comments.Sep 29 2017, 1:27 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10705 ↗(On Diff #117204)

I did not find any function to tell if a value is rounded to integer. Thinking about that function it might be non-trivial, given that we might have denorms. So I decided to go with opcodes which guarantied to produce integral value no matter what. A function like that most likely shall also check for these opcodes first before taking more complex analysis.

b-sumner edited edge metadata.Sep 29 2017, 1:32 PM

We could potentially update visitCEIL and visitFLOOR as well, and use the same opcode test in each, although I don't think such combinations are very likely.

We could potentially update visitCEIL and visitFLOOR as well, and use the same opcode test in each, although I don't think such combinations are very likely.

Right, that is possible but unlikely to happen. The current situation we have is due to the fact we have (fptosi (rint x)). I.e. we have rounded value but now we need to get integer from rounded float, and trunc is a part of that fptosi expansion.

We could potentially update visitCEIL and visitFLOOR as well, and use the same opcode test in each, although I don't think such combinations are very likely.

Right, that is possible but unlikely to happen. The current situation we have is due to the fact we have (fptosi (rint x)). I.e. we have rounded value but now we need to get integer from rounded float, and trunc is a part of that fptosi expansion.

Okay. That makes sense. Please add a comment noting that here (i.e. that trunc is part of the fpto[su]i expansions on some targets, so we're likely to generate this combination).

rampitec updated this revision to Diff 117223.Sep 29 2017, 2:24 PM

Added comment as requested.

hfinkel accepted this revision.Oct 1 2017, 6:38 AM

LGTM

This revision is now accepted and ready to land.Oct 1 2017, 6:38 AM
This revision was automatically updated to reflect the committed changes.