This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] define binops as a superset of commutative binops
ClosedPublic

Authored by spatel on May 21 2019, 5:58 AM.

Details

Summary

The test diffs show improved vector narrowing for integer min/max opcodes because those were all absent from the list. I'm not sure if we can expose functional diffs for all of the moved/added opcodes though.

It seems like we are missing an AVX512 opportunity to use 256-bit ops in place of 512-ops on some tests/targets, but I think that can be a follow-up.

Diff Detail

Event Timeline

spatel created this revision.May 21 2019, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2019, 5:58 AM
Herald added a subscriber: mcrosier. · View Herald Transcript
lebedev.ri accepted this revision.May 21 2019, 6:10 AM

Looks good to me.

This revision is now accepted and ready to land.May 21 2019, 6:10 AM
lebedev.ri added inline comments.May 21 2019, 6:23 AM
llvm/include/llvm/CodeGen/TargetLowering.h
2177

Actually, while this patch in itself looks good, this function looks a bit scary.
FP ops are commutative, but not associative..

spatel marked an inline comment as done.May 21 2019, 6:38 AM
spatel added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2177

Yes, that is scary. From what I can tell, we only call this via DAGCombiner::reassociateOps() starting from add/mul/and/or/xor. Add an assert on the value type to prevent misuse?

lebedev.ri marked an inline comment as done.May 21 2019, 6:43 AM
lebedev.ri added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2177

Asserts are great.

spatel marked an inline comment as done.May 21 2019, 7:54 AM
spatel added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2177

Digging in a bit more - if we have loose FP math, we might want to try the mild FP reassociations within DAGCombiner::reassociateOpsCommutative(). And we're already checking the node flags there, so I added a real check for FMF (even though it should currently never trigger):
rL361268

I'll let that bake a bit to see if there are any non-trunk complaints, then come back to this patch.

This revision was automatically updated to reflect the committed changes.