This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][RISCV][ARM][PowerPC][X86][WebAssembly] Change default abs expansion to use sra (X, size(X)-1); sub (xor (X, Y), Y).
ClosedPublic

Authored by craig.topper on Feb 7 2022, 11:27 AM.

Details

Summary

Previous we used sra (X, size(X)-1); xor (add (X, Y), Y).

By placing sub at the end, we allow RISCV to combine sign_extend_inreg
with it to form subw.

Some X86 tests for Z - abs(X) seem to have improved as well.

Other targets look to be a wash.

I had to modify ARM's abs matching code to match from sub instead of
xor. Maybe instead ISD::ABS should be made legal. I'll try that in
parallel to this patch.

This is an alternative to D119099 which was focused on RISCV only.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 7 2022, 11:27 AM
craig.topper requested review of this revision.Feb 7 2022, 11:27 AM

clang-format

craig.topper added inline comments.Feb 7 2022, 11:30 AM
llvm/test/CodeGen/Thumb/optionaldef-scheduling.ll
1–9

This test is likely no longer testing what it was before and I don't know how to fix it.

jrtc27 added inline comments.Feb 7 2022, 11:35 AM
llvm/test/CodeGen/Thumb/optionaldef-scheduling.ll
1–9

Presumably should be a MIR test that runs just the machine scheduler on a previously-broken instruction sequence?..

llvm/test/CodeGen/X86/abs.ll
28–29

Looks like we picked up an instruction here?

llvm/test/CodeGen/X86/iabs.ll
24–25

... and here

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/basic.ll.expected
1

Heh, maybe basic.ll should be even more basic...

craig.topper added inline comments.Feb 7 2022, 11:48 AM
llvm/test/CodeGen/X86/abs.ll
28–29

Yeah. The ADD we had before could be converted to LEA. But we can't do that for sub. i16 and above use cmov, but we have no i8 cmov.

FWIW, the new code matches gcc.

I think it's less likely to be an issue when we don't have a register allocation constraint from the top and bottom of the sequence.

If we're really concerned I an try custom selecting this back the old way for i8?

tlively added inline comments.Feb 7 2022, 12:10 PM
llvm/test/CodeGen/WebAssembly/PR41149.ll
17–18

Wasm change LGTM

The x86 diffs seem unlikely to make much difference in real code, so this change seems fine to me. Adding some more potential x86 reviewers in case someone wants to run benchmarks to confirm.

I've no objections to using this instead of D119099, even with the x86 i8 codegen change

llvm/test/CodeGen/RISCV/rv64zbb.ll
964

update FIXME?

Updated FIXME

I looked into improving ARM's abs matching. If I make ISD::ABS legal, then we change tests for nabs patterns. Those currently get expanded to "sra (X, size(X)-1); sub (Y, xor (X, Y))" through a generic DAG combine. If abs is legal that doesn't happen. Instead we'll have "(neg (abs X))". That will get expanded to the ABS or t2ABS pseudo instruction followed by a negation. Perhaps it would be better to have a NABS pseudo that expands to a different branch condition than the ABS pseudo.

Ping. Anyone have other comments?

RKSimon accepted this revision.Feb 20 2022, 11:21 AM

LGTM cheers - please can either add a TODO or raise a bug for better ARM/AArch64 ABS+NABS selection

This revision is now accepted and ready to land.Feb 20 2022, 11:21 AM
This revision was landed with ongoing or failed builds.Feb 20 2022, 9:44 PM
This revision was automatically updated to reflect the committed changes.