Page MenuHomePhabricator

[InstCombine] use select-of-constants with set/clear bit mask patterns
ClosedPublic

Authored by spatel on Apr 26 2020, 9:14 AM.

Details

Summary

Cond ? (X & ~C) : (X | C) --> (X & ~C) | (Cond ? 0 : C)
Cond ? (X | C) : (X & ~C) --> (X & ~C) | (Cond ? C : 0)

The select-of-constants form results in better codegen. There's an existing test diff that shows a transform that results in an extra IR instruction, but that's an existing problem.

This is motivated by code seen in LLVM itself - see PR37581:
https://bugs.llvm.org/show_bug.cgi?id=37581

define i8 @src(i8 %x, i8 %C, i1 %b)  {
  %notC = xor i8 %C, -1
  %and = and i8 %x, %notC
  %or = or i8 %x, %C
  %cond = select i1 %b, i8 %or, i8 %and
  ret i8 %cond
}

define i8 @tgt(i8 %x, i8 %C, i1 %b)  {
  %notC = xor i8 %C, -1
  %and = and i8 %x, %notC
  %mul = select i1 %b, i8 %C, i8 0
  %or = or i8 %mul, %and
  ret i8 %or
}

http://volta.cs.utah.edu:8080/z/Vt2WVm

Diff Detail

Event Timeline

spatel created this revision.Apr 26 2020, 9:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 9:14 AM
nikic accepted this revision.May 2 2020, 2:22 AM

LGTM

This revision is now accepted and ready to land.May 2 2020, 2:22 AM
This revision was automatically updated to reflect the committed changes.

Hi, we're seeing a small (1.0%) regression in omnetpp_r in SPEC INT 2017 on AArch64 with LTO enabled that bisects to this patch. I should be able to reduce omnetpp_r to a small IR example that shows the changed AArch64 codegen, if that's useful. A revert is probably not necessary if all we need is an additional pattern or two in the AArch64 backend.

spatel added a comment.May 6 2020, 8:07 AM

Hi, we're seeing a small (1.0%) regression in omnetpp_r in SPEC INT 2017 on AArch64 with LTO enabled that bisects to this patch. I should be able to reduce omnetpp_r to a small IR example that shows the changed AArch64 codegen, if that's useful. A revert is probably not necessary if all we need is an additional pattern or two in the AArch64 backend.

Thanks for letting me know. Yes, an IR example would be very helpful in deciding how to proceed.
Note: D68911 has been sitting untouched for ~6 months...

Do you think D68911 has a good chance of helping here? I can do a quick test run (quicker than finding a good reproducer) to see if improves.

spatel added a comment.May 6 2020, 9:23 AM

Do you think D68911 has a good chance of helping here? I can do a quick test run (quicker than finding a good reproducer) to see if improves.

I can't say for sure, but it was the 1st thing that came to mind, and yes, it should be easy to run perf tests.