This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Minor optimization for bswap with binary ops
ClosedPublic

Authored by RKSimon on Nov 25 2014, 7:19 AM.

Details

Summary

Added inst combine patterns for BSWAP with and/or/xor OP:

OP( BSWAP(x), BSWAP(y) ) -> BSWAP( OP(x, y) )
OP( BSWAP(x), CONSTANT ) -> BSWAP( OP(x, BSWAP(CONSTANT) ) )

Fixes PR15782

Since its just a one liner, I've also added BSWAP to the DAGCombiner equivalent as well:

fold (OP (bswap x), (bswap y)) -> (bswap (OP x, y))

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 16619.Nov 25 2014, 7:19 AM
RKSimon retitled this revision from to [InstCombine] Minor optimization for bswap with binary ops.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon added reviewers: qcolombet, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
qcolombet accepted this revision.Dec 1 2014, 1:02 PM
qcolombet edited edge metadata.

Hi Simon,

This LGTM with a couple of minor fixes. See my inline comments.

Also, for the tests, please use opt -instnamer on the input to rename the variables. Indeed, having numbered variables makes the tests harder to update.
Moreover, I know that this tests check that we end up with no bswap. To this purpose, you added a bswap at the end of each of the computation. I believe that it complicates the test cases with no value. I think it would be best to use simpler test cases with CHECK lines, but you will have to fix all the tests.
I leave that up to you and it can be done as a subsequent commit.

Thanks,
-Quentin

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
148 ↗(On Diff #16619)

Use early exit.

test/Transforms/InstCombine/bswap-fold.ll
70 ↗(On Diff #16619)

Get rid of the #0.

75 ↗(On Diff #16619)

Add a new line.

This revision is now accepted and ready to land.Dec 1 2014, 1:02 PM
RKSimon closed this revision.Dec 4 2014, 1:44 AM
RKSimon updated this revision to Diff 16916.

Closed by commit rL223349 (authored by @RKSimon).

deeprobin added a subscriber: deeprobin.EditedMar 5 2022, 2:18 PM

@RKSimon

Do you have an example in which productive case this optimization is useful?
No developer will intentionally create two bswaps in a row.

Or was this PR just a nice-to-have?

/cc @qcolombet
/cc @EgorBo

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 5 2022, 2:18 PM

@RKSimon

Do you have an example in which productive case this optimization is useful?
No developer will intentionally create two bswaps in a row.

Or was this PR just a nice-to-have?

/cc @qcolombet
/cc @EgorBo

Example code in PR:

https://bugs.llvm.org/show_bug.cgi?id=15782

8 year old patch…

No developer will …

Very strong words :D

@RKSimon

Do you have an example in which productive case this optimization is useful?
No developer will intentionally create two bswaps in a row.

Or was this PR just a nice-to-have?

/cc @qcolombet
/cc @EgorBo

@xbolva00 @RKSimon

8 year old patch…

Yes I would be interested in the real-world case behind this.

No developer will …

Very strong words :D

The question is whether this is a niche optimization or whether there is some common code pattern that justifies that this is not a niche optimization.

@xbolva00 @RKSimon

8 year old patch…

Yes I would be interested in the real-world case behind this.

No developer will …

Very strong words :D

The question is whether this is a niche optimization or whether there is some common code pattern that justifies that this is not a niche optimization.

Perhaps it would be more productive to approach this from another angle - what prompted you to bring this up?

Sorry for not replying (I tend to assume any comments on ancient phabs are spambots).

Developers are human too - they might not intentionally do many things, but they still crop up!

IIRC (8 years...........) as well as PR15782, this particular case turned up in some ABGR -> RGBA pixel code swizzling. I can imagine it potentially happening in network 'ntohl' style ops as well.

@xbolva00 @RKSimon

8 year old patch…

Yes I would be interested in the real-world case behind this.

No developer will …

Very strong words :D

The question is whether this is a niche optimization or whether there is some common code pattern that justifies that this is not a niche optimization.

Perhaps it would be more productive to approach this from another angle - what prompted you to bring this up?

I generally have different scenarios in C# (even rare ones) and came up with that the .NET runtime does not optimize duplicate bswaps.
In the runtime niche optimizations are done rather rarely, so I wanted to see for what reason LLVM has this built in.

See https://github.com/dotnet/runtime/issues/66249

Sorry for not replying (I tend to assume any comments on ancient phabs are spambots).

Developers are human too - they might not intentionally do many things, but they still crop up!

All is well :D

IIRC (8 years...........) as well as PR15782, this particular case turned up in some ABGR -> RGBA pixel code swizzling. I can imagine it potentially happening in network 'ntohl' style ops as well.

Thank you very much. I think that helps me already.

@xbolva00 @RKSimon

8 year old patch…

Yes I would be interested in the real-world case behind this.

No developer will …

Very strong words :D

The question is whether this is a niche optimization or whether there is some common code pattern that justifies that this is not a niche optimization.

Perhaps it would be more productive to approach this from another angle - what prompted you to bring this up?

I generally have different scenarios in C# (even rare ones) and came up with that the .NET runtime does not optimize duplicate bswaps.
In the runtime niche optimizations are done rather rarely, so I wanted to see for what reason LLVM has this built in.

See https://github.com/dotnet/runtime/issues/66249

That issue is about bswap(bswap(x)) --> x, this patch was for logic(bswap(x), bswap(y)) --> bswap(logic(x, y)).