Page MenuHomePhabricator

[AARCH64][DAGCombine] Add combine for negation of CSEL absolute value pattern.
ClosedPublic

Authored by sunho on Jan 31 2022, 3:54 AM.

Details

Reviewers
fhahn
dmgreen
Summary

Fixes https://github.com/llvm/llvm-project/issues/51558.

The changes were ported from https://reviews.llvm.org/D111858 with some differences specific to aarch64 backend.

Diff Detail

Event Timeline

sunho created this revision.Jan 31 2022, 3:54 AM
sunho requested review of this revision.Jan 31 2022, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2022, 3:54 AM
sunho set the repository for this revision to rG LLVM Github Monorepo.
sunho updated this revision to Diff 404476.Jan 31 2022, 4:00 AM

Edit typos

sunho retitled this revision from [AArch64][DAGCombine] Add combine for negation of CSEL absolute value pattern. to [AARCH64][DAGCombine] Add combine for negation of CSEL absolute value pattern..Jan 31 2022, 4:01 AM
sunho updated this revision to Diff 404479.Jan 31 2022, 4:20 AM

Edit existing test

Hello. This sounds good. It is similar to the patch in D112204 (but I din't have any time to continue that, glad to see you looking into the same thing!)

Feel free to steal what is useful from it, if you thing the suggested simplification is a good idea.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14699

I think we might be able to generalize this a bit, possibly simplifying it in the process. If we have neg (csel x, y, cc), we can convert to csel (neg x), (neg y), cc, and providing at least one of x and y simplify it will be the same cost or cheaper (with a csneg operation).

I think that means that the condition is unimportant, and we won't need to emit the Add. It won't handle all the same patterns this does, both might be quite rare for cases that are not -abs (as llvm will have often optimized before this point already), but might be more straight forward.

17667

I think you can include this in performAddSubCombine, just with a check that the opcode is ISD::SUB in there or performCombineSubABS.

sunho added a comment.EditedFeb 2 2022, 11:24 AM

I like your method! It's always reducing negation or keeping the same number of negation unless x and y are both non-negative and it can account for nabs case very well. I was trying to tidy your patch a bit and add some more test cases to address the comments by david-arm and submit it here. However, one thing came to concern me.

When one of x and y are negative maximum of that integer type (e.g. -128 for int8), then folding negation can nullify signed integer overflows that would have happended if not folded. For instance, if you have -csel(-x,-y), then it will be folded to csel(x,y). If x was the negative maximum of that integer type, it would lead to singed integer overflow in the original form (-(-128)), but it will not after folded.

I'm not sure whether I can just consider this case UB and fold it. Could you give me some advice on this?

sunho added a comment.EditedFeb 2 2022, 12:52 PM

IIUC, if nsw flag is not specified it will wrap around, then -(-128) = -128, still yielding the same result. Even if nsw flag is specified, I can define poison value as anything I want so it's still fine?

sunho added a comment.Feb 2 2022, 1:24 PM

https://alive2.llvm.org/ce/z/2KoTdr It always passes unless I put nsw on sub in tgt block. I guess that means it's okay.

sunho updated this revision to Diff 405437.Feb 2 2022, 2:06 PM

Use simpler pattern based on https://reviews.llvm.org/D112204

sunho updated this revision to Diff 405438.Feb 2 2022, 2:07 PM

Remove space.

sunho added inline comments.Feb 2 2022, 2:17 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14730

I've added isNegatedInteger util function as suggested by david-arm in the original patch. The util function can be used in many places across multiple backends. I can submit a follow-up patch replacing to use this function if desired.

sunho marked an inline comment as not done.Feb 2 2022, 2:17 PM
dmgreen added inline comments.Feb 7 2022, 4:08 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14699

SDValues can be passed by value - they are pretty small.

14718

Could this now use: if (!isNegatedInteger(N))?

llvm/test/CodeGen/AArch64/neg-selects.ll
1

Can you pre-commit the tests and show just the differences here?

sunho updated this revision to Diff 410168.Feb 20 2022, 10:07 AM

Sorry for the delay. I've separated commits that adds tests to https://reviews.llvm.org/D120214, and make this commit only show the difference between older version.

dmgreen accepted this revision.Feb 21 2022, 12:32 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Feb 21 2022, 12:32 AM

Cheers! Can you commit on my behalf?

Author name: sunho
Author email: ksunhokim123@gmail.com