This is an archive of the discontinued LLVM Phabricator instance.

Allow bitwidth difference when checking for isOneOrOneSplat.
ClosedPublic

Authored by adriantong1024 on Jun 8 2022, 3:10 PM.

Details

Summary
This helps handling a case where the BUILD_VECTOR has i16 element type
and i32 constant operands

t2: v8i16 = setcc t8, t17, setult:ch
t3: v8i16 = BUILD_VECTOR Constant:i32<1>, ...
   t4: v8i16 = and t2, t3
      t5: v8i16 = add t8, t4

This can be turned into t5: v8i16 = sub t8, t2, and allows us to remove
t3 and t4 from the DAG.

Diff Detail

Event Timeline

adriantong1024 created this revision.Jun 8 2022, 3:10 PM
adriantong1024 requested review of this revision.Jun 8 2022, 3:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 3:10 PM
craig.topper added inline comments.Jun 8 2022, 3:15 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15422 ↗(On Diff #435357)

No need to declare Known outside of the blocks they are used in.

15425 ↗(On Diff #435357)

Why Depth is 1 here? Shouldn't we just use the default which is 0?

Address comments.

craig.topper added inline comments.Jun 8 2022, 3:37 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15425 ↗(On Diff #435362)

Declare Known on the line where it is assigned.

KnownBits Known = DAG.computeKnownBits...
15432 ↗(On Diff #435362)

This is no inconsistent with the other call to computeKnownBits. I think for both you can do DAG.computeKnownBits(LHS) or DAG.computeKnownBits(RHS).

This shoudl have been picked up by foldAddSubMasked1 in the generic DAG combiner but wasn't because the BUILD_VECTOR has i8 element type and i32 constant operands. We should fix that to not require the widths to match.

This shoudl have been picked up by foldAddSubMasked1 in the generic DAG combiner but wasn't because the BUILD_VECTOR has i8 element type and i32 constant operands. We should fix that to not require the widths to match.

Oh. let me look into it. Thanks for the pointer

Address comments from Craig. Thanks for the pointer, this does simplify things.

adriantong1024 retitled this revision from Implement capability to optimize add negative into subtract positive in AArch64. to Allow bitwidth difference when checking for isOneOrOneSplat..Jun 14 2022, 8:32 AM
adriantong1024 edited the summary of this revision. (Show Details)
craig.topper added inline comments.Jun 14 2022, 8:38 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
10664–10665

Are all of the callers of isOneOrOneSplat ok with this change?

Is this change correct for the caller you’re interested in if the element type of the build_vector is i1?

adriantong1024 added inline comments.Jun 14 2022, 8:53 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
10664–10665

Let me go through the callers one by one. There are 8 uses of isOneOrOneSplat in llvm, all in DAGCombiner.cpp.

so far, I ran through check-llvm and looks good.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
10664–10665

The bitwidth check was added https://reviews.llvm.org/D25374 and I have ran check-llvm-codegen for all available targets and things are fine.

RKSimon added inline comments.
llvm/test/CodeGen/AArch64/add-negative.ll
2

Remove " --check-prefixes=CHECK" - its the default

19

Please can you pre-commit this and rebase so the patch shows the diff?

Address the comments from @RKSimon. Thanks!
And rebase.

I am unable to reproduce the failures in check-openmp locally.

RKSimon added inline comments.Jun 15 2022, 9:50 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
10664–10665

This comment can probably go - unlike Null/AllOnes this isn't going to work well if the bitcast is to a vector type with different scalar sizes

Address comment from Simon. Thanks!

I ran ninja check-lld check-libc check-flang check-llvm check-polly check-bolt check-mlir check-all check-clang check-clang-tools check-openmp locally and things are good.

RKSimon accepted this revision.Jun 16 2022, 3:08 AM

LGTM

This revision is now accepted and ready to land.Jun 16 2022, 3:08 AM
This revision was landed with ongoing or failed builds.Jun 16 2022, 9:05 AM
This revision was automatically updated to reflect the committed changes.