Page MenuHomePhabricator

[SelectionDAG] Add pattern to haveNoCommonBitsSet.
Needs ReviewPublic

Authored by OmerAviram on Tue, Nov 16, 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.Tue, Nov 16, 12:54 AM
OmerAviram requested review of this revision.Tue, Nov 16, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 16, 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.Wed, Nov 17, 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.Wed, Nov 17, 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.