[PATCH] D62191: [SelectionDAG] define binops as a superset of commutative binops

Tue May 21 07:55:48 PDT 2019

spatel marked an inline comment as done.
spatel added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2199
   /// Returns true if the opcode is a commutative binary operation.
   virtual bool isCommutativeBinOp(unsigned Opcode) const {
     // FIXME: This should get its info from the td file.
----------------
lebedev.ri wrote:
> spatel wrote:
> > lebedev.ri wrote:
> > > Actually, while this patch in itself looks good, this function looks a bit scary.
> > > FP ops are commutative, but not associative..
> > > 
> > 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?
> Asserts are great.
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62191/new/

https://reviews.llvm.org/D62191