Page MenuHomePhabricator

[Reassociation] Add support for reassociation with unsafe algebra.
ClosedPublic

Authored by mcrosier on Jun 12 2014, 2:37 PM.

Details

Summary

The purpose of this patch is to add support for reassociation with unsafe algebra.

I've performed correctness testing on AArch64 with our internal test suite and saw no correctness regressions. I'm currently running correctness against ARMv7.

The performance improvements look good, but I'm unable to share the numbers at this time; I'll see what I can do(, but no promises).

Please have a look!

Chad

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 10372.Jun 12 2014, 2:37 PM
mcrosier retitled this revision from to [Reassociation] Add support for reassociation with unsafe algebra. .
mcrosier updated this object.
mcrosier edited the test plan for this revision. (Show Details)
mcrosier set the repository for this revision to rL LLVM.
chandlerc edited edge metadata.Jun 12 2014, 3:09 PM

Is there a reason this is not CC-ed to the list?

mcrosier added a subscriber: Unknown Object (MLST).Jun 12 2014, 3:16 PM

No, this should have been sent to the list as well!

mcrosier edited edge metadata.Jun 12 2014, 3:17 PM
chandlerc added inline comments.Jun 12 2014, 3:17 PM
lib/Transforms/Scalar/Reassociate.cpp
362

Why a local variable above but not here? I don't care deeply about which pattern you use, but I would generally avoid the single-use variable unless things get truly terrible to read...

854

The more I see of code like this, the more I think we need a richer clone approach in instructions... but that's not for this patch...

901–902

This formatting I find hard to read... clang-format?

914

Are you actually intending to use this variable outside of the if? How (as its null)? See comment below...

963

You pass BI twice here... is that intentional? Was one of them supposed to be I?

1951–1952

Why not floating point vector instructions? That seems rather odd to group with testing for fastmath...

mcrosier added inline comments.Jun 12 2014, 3:31 PM
lib/Transforms/Scalar/Reassociate.cpp
362

I agree. I'll take care of it.

901–902

Will do.

914

No. I'll put this in the canonical form. Thanks.

if (BinaryOperator *I = isReassociableOp(V, Instruction::Add, Instruction::FAdd)) {

}

963

Unfortunately, it's intentional (for now). The 3rd argument in the insertion point and the 4th is where FastMathFlags should be copied from. For CreateMul and CreateAdd these two arguments are always the same. However, for FNeg theres one instance where these two arguments are different (i.e., Line 1129). I could pass the FPMathFlags in, but then I would be hoisting the type checking out of the static helper functions, which would defeat the purpose.

1951–1952

I've already addressed this internally, but I need to rerun correctness and performance.

mcrosier added inline comments.Jun 16 2014, 1:55 PM
lib/Transforms/Scalar/Reassociate.cpp
1951–1952

Turns out that vector instructions are not handled for either integer for floating point instructions. If you don't mind, I'd prefer to land that change as a separate patch.

mcrosier updated this revision to Diff 12141.Aug 2 2014, 7:24 AM

Revised patch based on Chandler's comments. This adds support for scalar floating-point instructions with unsafe math only (i.e., no vector support, yet).

Please have a look!

Chad

If someone would mind reviewing this patch, I would greatly appreciate it. It's fairly straight forward to understand.

Please! :D

Chad

p.s. Please!

How about a post-commit review, given Chandler has already given this an initial review?

*tap* *tap* *tap* ...is this thing on? :)

Help me out, please.

Chad

grosbach edited edge metadata.Aug 13 2014, 10:17 AM

I'm fine with that if Chandler is. It all seems reasonable to me, but I'm not a floating point expert.

Thanks, Jim. Hopefully, Chandler or someone else will chime in.

Sorry I kept failing to get back to this.

Feel free to submit whenever. See comments below for my only (minor) comments. I don't see anything needing to wait on more rounds of review though.

I look forward to seeing the next 3 things I see here:

  1. Support swapping the operand order once SLP vectorizer is OK with it
  2. Support for floating point vectors
  3. (maybe?) Support for integer vectors?
lib/Transforms/Scalar/Reassociate.cpp
884

Why not do this before or ofter the if, but outside of the if? It's duplicated in both sides.

963

I see. This makes sense. On looking at things more carefully, I actually like the pattern you use here more than the pattern for CreateMul and CreateAdd. It seems strange to conflate "insertion point" and "fast math flags source". Is there some conceptual reason to have them be the same? If not, I'd split them in all the methods.

1965–1968

I would really, really like to know why on earth we don't reassociate integer vector operations. That seems... kind of crazy.

chandlerc accepted this revision.Aug 14 2014, 2:10 AM
chandlerc edited edge metadata.
This revision is now accepted and ready to land.Aug 14 2014, 2:10 AM
  1. Support swapping the operand order once SLP vectorizer is OK with it I believe Erik addressed this here: http://llvm.org/viewvc/llvm-project?view=revision&revision=214485
  1. Support for floating point vectors
  2. (maybe?) Support for integer vectors?

I have a follow up patch that addresses both of these.

lib/Transforms/Scalar/Reassociate.cpp
884

The SubclassOptionalData contains the FastMathFlags, so we need to save and restore that state before we call clearSubclassOptionalData(). Also, the calls to getFastMathFlags() and setFastMathFlags() needs to be guarded by the isa<FPMathOperator> check. Otherwise, we hit an assert.

1968

I agree; I have a follow up patch that will take care of this..

mcrosier closed this revision.Aug 14 2014, 9:43 AM

Committed r215647.