Page MenuHomePhabricator

[SLP] Remove LHS and RHS from OperationData.
ClosedPublic

Authored by craig.topper on Wed, Sep 23, 6:28 PM.

Details

Summary

These were only really used for 2 things. One was to check if the operand matches the phi if it exists. The other was for the createOp method to build the reduction.

For the first case we still have the operation we just need to know how to index its operands. So I've modified getLHS/getRHS to just use the opcode/kind to know how to find the right operands on an instruction that is now passed in.

For the other case we had to create an OperationData object to set the LHS/RHS values and copy the opcode/kind from another object. We would then just call createOp on that temporary object. Instead I've made LHS/RHS arguments to createOp and removed all these temporary objects.

Ultimately, I'm just trying to familiarize myself with this code so I can figure how fast math flags should work. Thought I'd try make some improvements along the way.

Diff Detail

Event Timeline

craig.topper created this revision.Wed, Sep 23, 6:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 23, 6:28 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Wed, Sep 23, 6:28 PM
craig.topper added inline comments.Wed, Sep 23, 6:36 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6484

I'm not entirely sure what this assert was trying to check before.

As far as I can tell there are 3 states OperationData could have

  • Kind is RK_None and Opcode is 0 and LHS/RHS wre.
  • Kind is RK_None and Opcode is non-zero and LHS/RHS are null
  • Kind is not RK_None and Opcode is non-zero and LHS/RHS are non-null

So I think this was previously saying that if the Kinds are equal then the LHS/RHS in both should be null or non-null. Maybe I should have changed it to (Kind != OD.Kind) || ((Opcode != 0) == (OD.Opcode != 0))?

6498

I remove this pointer comparison. I didn't see any reason checking the fields wasn't ok this and OD being the same object.

ABataev accepted this revision.Thu, Sep 24, 6:02 AM

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6484

I think this assert is good in its current form.

This revision is now accepted and ready to land.Thu, Sep 24, 6:02 AM
This revision was automatically updated to reflect the committed changes.