Page MenuHomePhabricator

[SelectionDAG] Add pattern to haveNoCommonBitsSet.
ClosedPublic

Authored by OmerAviram on Nov 16 2021, 12:54 AM.

Details

Summary

Correctly identify the following pattern, which has no common bits: (X & ~M) op (Y & M).

Diff Detail

Event Timeline

OmerAviram created this revision.Nov 16 2021, 12:54 AM
OmerAviram requested review of this revision.Nov 16 2021, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 12:54 AM

Added ll test for X86 Target.
prior to the patch, the ir included an or instruction:

; CHECK-NEXT:    andl %esi, %edx
; CHECK-NEXT:    notl %esi
; CHECK-NEXT:    andl %edi, %esi
; CHECK-NEXT:    orl %edx, %esi
; CHECK-NEXT:    leal 1(%rsi), %eax

After the patch, the or instruction can be combined into an add, since the operands have no common bits.

RKSimon added inline comments.Nov 17 2021, 1:54 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4552

Maybe better to pull this into haveNoCommonBitsSet as lambda?

4554

You need to try and ensure we have test coverage for all of these permutations.

llvm/test/CodeGen/X86/or-lea.ll
155

Once you have more test coverage, we should add these tests to trunk with current codegen so you can rebase and the patch shows the diff.

Added tests for the different pattern permutations as a separate patch (https://reviews.llvm.org/D114078) and rebased on it.

OmerAviram marked 3 inline comments as done.Nov 17 2021, 5:00 AM

Updated diff with i32 and i64 tests.

Please can you rebase - I've added BMI test coverage as well which might affect some tests

Do we expect all vector types/targets to bypass this in some way? If yes, it's worth adding a comment to the patch description and/or code. If no, can we find a test that shows an asm difference?

Do we expect all vector types/targets to bypass this in some way? If yes, it's worth adding a comment to the patch description and/or code. If no, can we find a test that shows an asm difference?

What exactly do you mean by bypass it? This addition to haveNoCommonBitsSet works for vector types as well. Do you mean add another lit test for vector types?

Do we expect all vector types/targets to bypass this in some way? If yes, it's worth adding a comment to the patch description and/or code. If no, can we find a test that shows an asm difference?

What exactly do you mean by bypass it? This addition to haveNoCommonBitsSet works for vector types as well. Do you mean add another lit test for vector types?

I mean are there TLI hooks (for example hasAndNot) or other transforms that will trigger before or after this such that vector instructions are never affected?
It sounds like the answer is 'no'. Therefore, yes we should have another lit test with vector types, so we can see the difference from this patch.

Added tests for vector types.

spatel added inline comments.Nov 29 2021, 7:03 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4550

I prefer that we not reuse/shadow the variable names in here.
How about matching the variable names to the code comment, so something like this:

auto MatchNoCommonBitsPattern = [&](SDValue NotM, SDValue And) {
  if (isBitwiseNot(NotM, true)) {
    SDValue M = NotM->getOperand(0);
OmerAviram marked an inline comment as done.
RKSimon added inline comments.Nov 29 2021, 8:48 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4554

(style) Remove the unnecessary brackets

spatel accepted this revision.Nov 29 2021, 8:56 AM

LGTM - will wait at least a day to commit in case there's any other feedback.

This revision is now accepted and ready to land.Nov 29 2021, 8:56 AM
OmerAviram marked an inline comment as done.

LGTM - will wait at least a day to commit in case there's any other feedback.

Thanks, Could you please commit on my behalf?

This revision was automatically updated to reflect the committed changes.