This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Enable more reassociations using FMF 'reassoc' + 'nsz'
ClosedPublic

Authored by wristow on May 24 2018, 10:19 AM.

Details

Summary

Reassociation of math ops in some contexts (especially vector contexts)
has generally only been happening when the 'fast' FMF was set. This
enables reassoication when only the finer grained controls 'reassoc' and
'nsz' are set.

This also improves many vector reassociation opportunities. It also improves some scalar reassociation, although I haven't looked for tests that can be made more aggressive on the scalar side yet (or written new tests on the scalar side).

This changes Instruction::isAssociative() to use hasAllowReassoc() && hasNoSignedZeros() rather than isFast() for FMul and FAdd ops. I believe that ultimately Instruction::isAssociative() should only check hasAllowReassoc() for these ops, and that transformations that also require nsz should be handled by the callers, as appropriate. Auditing all the callers would be needed to get that correct. I think this improves the situation, without creating a danger.

Diff Detail

Repository
rL LLVM

Event Timeline

wristow created this revision.May 24 2018, 10:19 AM
spatel accepted this revision.May 24 2018, 12:14 PM

Technically, this isn't only an instcombine change.
I was surprised that we didn't get any test diffs for -reassociate, but I see now that it also has internal checks for 'isFast', so we won't get any progress there until those predicates are changed too.

LGTM.

This revision is now accepted and ready to land.May 24 2018, 12:14 PM
mcberg2017 accepted this revision.May 24 2018, 12:19 PM

I have a note to look at this relaxed with my D46562 to see is we can loose nsz yet.

LGTM

This revision was automatically updated to reflect the committed changes.

Thanks for the quick reviews!
Regarding:

Technically, this isn't only an instcombine change.

I left the [InstCombine] tag in the commit message, since this currently is impacting mostly (maybe only) -instcombine. For a moment, I considered [InstCombine,Reassociate], but decided to leave it as-is.