This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Respect SDNodeFlags in lowering SELECT_CC
ClosedPublic

Authored by qiucf on Apr 29 2020, 2:09 AM.

Details

Summary

Legalizer should respect both command-line options or SDNode-level fast-math flags.

Also, this patch propagates other flags during custom simplifying.

Diff Detail

Event Timeline

qiucf created this revision.Apr 29 2020, 2:09 AM
qiucf marked an inline comment as done.Apr 29 2020, 2:13 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/scalar-min-max.ll
129

Deleting fast in this file looks okay to me, since we already used command-line flags. We can clear this in following patches.

steven.zhang accepted this revision.May 10 2020, 8:57 PM

Can we not to remove the --enable-unsafe-fp-math in this patch ? And I think, the option --enable-unsafe-fp-math is also part of our test point.

This revision is now accepted and ready to land.May 10 2020, 8:57 PM
qiucf planned changes to this revision.May 10 2020, 9:01 PM

Not remove --enable-unsafe-fp-math

qiucf updated this revision to Diff 263353.May 11 2020, 11:04 PM

Keep flags and options as-is, so that we can have a clear sight of what changed. Options/flags will be updated in child revision.

This revision is now accepted and ready to land.May 11 2020, 11:04 PM
steven.zhang accepted this revision.May 11 2020, 11:50 PM

LGTM now.

llvm/test/CodeGen/PowerPC/scalar_cmp.ll
129–132

These instructions sequence seem terrible to me, as we didn't have INF and NAN for this case. But it is not relative with your change, maybe, worth to take a look.

This revision was automatically updated to reflect the committed changes.
qiucf marked an inline comment as done.May 13 2020, 1:55 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/scalar_cmp.ll
129–132

hmm.. This is expected result since we propagate these flags to SELECT_CC now. Do you mean that sub-sub here is worse than cmp-beq-fmr?

steven.zhang added inline comments.May 13 2020, 3:24 AM
llvm/test/CodeGen/PowerPC/scalar_cmp.ll
129–132

I think so, we have two subs + two fsel. Not sure why we do it that way.