This is an archive of the discontinued LLVM Phabricator instance.

[Reassocate] Add support for vector instructions.
AbandonedPublic

Authored by mcrosier on Sep 5 2014, 5:30 PM.

Details

Summary

As the title suggests, this patch adds support for vector instructions in the reassociation pass. This includes both integer and floating-point (with unsafe algebra) vector instructions. This patch is a WIP, but I thought it was in a stable enough state to begin the review process.

If anyone has vector workloads, I would really like to know if they benefit from this patch. I didn't see any real benefit on the EEMBC or SPEC suites.

Regards,

Chad

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 13353.Sep 5 2014, 5:30 PM
mcrosier retitled this revision from to [Reassocate] Add support for vector instructions..
mcrosier updated this object.
mcrosier edited the test plan for this revision. (Show Details)
mcrosier added a subscriber: Unknown Object (MLST).
spatel edited edge metadata.Sep 9 2014, 3:22 PM

Hi Chad -

Thanks for posting this patch. I wasn't able to apply the patch cleanly to r217455, so I'm not sure if that is affecting what I'm seeing.

I think @test20 in test/Transforms/Reassociate/vector-basictests.ll should cover what I'm trying to do in DAGCombiner here:
http://reviews.llvm.org/D5254

It seems to work when there's a 'fast' directly on the fmuls, but not if I specify "unsafe-fp-math=true" as a function attribute or via the "enable-unsafe-fp-math" parameter to opt. I guess this a general question: what are we supposed to be using to detect -ffast-math and friends? Do we have to consider both the IR flags and the attributes/params?

spatel added a comment.Sep 9 2014, 3:44 PM

On closer inspection, -instcombine is handling all of the transforms in the fmul testcases that I added in the other patch, so the changes to -reassociate with this patch aren't coming into play at all...but only if 'fast' is specified on each fmul; the function attribute appears to be ignored.

In D5222#5, @spatel wrote:

Hi Chad -

Thanks for posting this patch. I wasn't able to apply the patch cleanly to r217455, so I'm not sure if that is affecting what I'm seeing.

Argg... Like I said, the patch has been laying around for some time. I'll see if I can't rebase it soon and upload a new diff.

I think @test20 in test/Transforms/Reassociate/vector-basictests.ll should cover what I'm trying to do in DAGCombiner here:
http://reviews.llvm.org/D5254

It seems to work when there's a 'fast' directly on the fmuls, but not if I specify "unsafe-fp-math=true" as a function attribute or via the "enable-unsafe-fp-math" parameter to opt. I guess this a general question: what are we supposed to be using to detect -ffast-math and friends? Do we have to consider both the IR flags and the attributes/params?

Good question. When I added support for unsafe-math I checked the instruction FPMath flags directly and not the function attributes. That doesn't mean it couldn't work that way, I just didn't implement it that way. Does this effect anything other than simplifying lit test creation? If this is commonly done, please file a PR and I'll be happy to address it.

I also recall why I hadn't upstreamed the patch earlier. My test cases are derived from the scalar tests and some of those rely on InstCombine and other passes to cleanup after the Reassociation pass. Unfortunately, some of these optimizations are not supported in the vector versions, so those need to be investigated.

I'll try to update things in a few days. Thanks for looking at the patch, Sanjay.

Chad

In D5222#7, @mcrosier wrote:

Good question. When I added support for unsafe-math I checked the instruction FPMath flags directly and not the function attributes. That doesn't mean it couldn't work that way, I just didn't implement it that way. Does this effect anything other than simplifying lit test creation? If this is commonly done, please file a PR and I'll be happy to address it.

Here's what I see in clang: when you pass '-ffast-math', the IRBuilderBases's FastMathFlag has its UnsafeAlgebra bit set and that is used as a function attribute. The bit is then propagated to any created instruction in the function that is eligible (fmul, fadd, fsub, fdiv, frem) to have a 'fast' modifier. So in practice, it seems like we can use either variable to detect if '-ffast-math' was specified if you're only interested in optimizing those FP instructions, but I don't see an explicit guarantee or documentation about this.

I think the case where detecting a 'fast' flag on an instruction is inadequate is when we're looking to optimize an FP intrinsic, such as square root:

float reciprocal_square_root(float x) {
     return 1.0f / sqrtf(x);
}

With -ffast-math on, we should be able to turn that into 'rsqrtss' (x86) and a Newton-Raphson step, but there's no way to express 'fast' on an intrinsic in the IR, so we have to use a function-level attribute.

In D5222#8, @spatel wrote:

Here's what I see in clang: when you pass '-ffast-math', the IRBuilderBases's FastMathFlag has its UnsafeAlgebra bit set and that is used as a function attribute. The bit is then propagated to any created instruction in the function that is eligible (fmul, fadd, fsub, fdiv, frem) to have a 'fast' modifier. So in practice, it seems like we can use either variable to detect if '-ffast-math' was specified if you're only interested in optimizing those FP instructions, but I don't see an explicit guarantee or documentation about this.

Thanks for looking into how this all works.

I think the case where detecting a 'fast' flag on an instruction is inadequate is when we're looking to optimize an FP intrinsic, such as square root:

float reciprocal_square_root(float x) {
     return 1.0f / sqrtf(x);
}

With -ffast-math on, we should be able to turn that into 'rsqrtss' (x86) and a Newton-Raphson step, but there's no way to express 'fast' on an intrinsic in the IR, so we have to use a function-level attribute.

If and when the Reassociation pass starts playing with intrinsics, we should consider addressing this issue. However, until that time the only advantage I see is that it would remove a few characters from the lit tests.

hfinkel edited edge metadata.Oct 1 2014, 10:27 AM

Two further comments:

  1. Once you have the rebased patch, please post it with full context (http://llvm.org/docs/Phabricator.html)
  2. We should split the test cases so the primary tests depend only on the reassociate pass. However, you can also add tests to instcombine/GVN/etc. so that we make sure the end-to-end result has coverage.
In D5222#7, @mcrosier wrote:

Good question. When I added support for unsafe-math I checked the instruction FPMath flags directly and not the function attributes. That doesn't mean it couldn't work that way, I just didn't implement it that way. Does this effect anything other than simplifying lit test creation? If this is commonly done, please file a PR and I'll be happy to address it.

Hi Chad,
I think that the function-level attribute needs to be handled too.
I've filed http://llvm.org/bugs/show_bug.cgi?id=21291 based on the discussion with Hal in http://reviews.llvm.org/D5787.

chandlerc resigned from this revision.Mar 29 2015, 1:37 PM
chandlerc removed a reviewer: chandlerc.

Assuming that this patch has been subsumed in subsequent work, but add me back and ping the patch if you actually want me to look at this.

spatel resigned from this revision.Mar 29 2015, 3:28 PM
spatel removed a reviewer: spatel.

This patch was reborn as D7566.

mcrosier abandoned this revision.Mar 31 2015, 1:55 PM

As pointed out by Sanjay, this work has been taken over by Robert Lougher.

test/Transforms/Reassociate/fast-ReassociateVector.ll