This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] commute setcc operands to match a subtract
ClosedPublic

Authored by spatel on Jun 28 2019, 2:34 PM.

Details

Summary

If we have:

R = sub X, Y
P = cmp Y, X

...then flipping the operands in the compare instruction can allow using a subtract that sets compare flags.

Motivated by diffs in D58875 - not sure if this changes anything there, but this seems like a good thing independent of that.

I've never seen Lanai code before this, so I don't know whether that change is good, bad, or even correct.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jun 28 2019, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 2:34 PM

Apart from the Lanai code (which like @spatel I know very little about) everything LGTM

spatel added a comment.Jul 1 2019, 8:28 AM

Apart from the Lanai code (which like @spatel I know very little about) everything LGTM

I'm not finding any docs for this target other than the code in LLVM itself. Discussion about whether this target should be included in LLVM from when it first appeared in trunk:
http://lists.llvm.org/pipermail/llvm-dev/2016-February/095118.html

From commit activity, maybe only @jpienaar can say what is happening on that target?

FWIW, there's a more involved version of this transform already in IR (in instcombine although that seems misplaced to me) - see "swapMayExposeCSEOpportunities()".

Apart from the Lanai code (which like @spatel I know very little about) everything LGTM

I'm not finding any docs for this target other than the code in LLVM itself. Discussion about whether this target should be included in LLVM from when it first appeared in trunk:
http://lists.llvm.org/pipermail/llvm-dev/2016-February/095118.html

From commit activity, maybe only @jpienaar can say what is happening on that target?

FWIW, there's a more involved version of this transform already in IR (in instcombine although that seems misplaced to me) - see "swapMayExposeCSEOpportunities()".

There is also http://g.co/lanai/isa . I'll try to patch and see what is happening with this change, the test seems suspicious given the change in select op without change to sub.f operands.

llvm/test/CodeGen/Lanai/sub-cmp-peephole.ll
28 ↗(On Diff #207144)

The select compare changes, but the operands to the subtract doesn't, that seems weird.

spatel marked an inline comment as done.Jul 2 2019, 8:14 AM
spatel added inline comments.
llvm/test/CodeGen/Lanai/sub-cmp-peephole.ll
28 ↗(On Diff #207144)

If I'm reading this asm correctly based on the linked doc:
http://g.co/lanai/isa

We have:
%a == %r6 , %b == %r7
so "sub.f %r7, %r6, %r3" means "b-a" goes into %r3
And "sel.gt %r3, %r0, %rv" means "b-a" goes into "rv" if "b-a" was greater than 0.

That matches the IR logic, so is the code miscompiled without this patch? If we have "sel.lt", then that means the returned value is negative or zero rather than the intended positive or zero?

jpienaar accepted this revision.Jul 10 2019, 1:46 PM

Lanai side looks good to me, thanks!

llvm/test/CodeGen/Lanai/sub-cmp-peephole.ll
28 ↗(On Diff #207144)

You are correct.

This revision is now accepted and ready to land.Jul 10 2019, 1:46 PM
This revision was automatically updated to reflect the committed changes.