This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] lower abs intrinsic to new ABS instruction in GIsel
ClosedPublic

Authored by stuij on Dec 6 2022, 5:32 AM.

Details

Summary

When feature CSSC is available, the abs intrinsic should map to the
new scalar ABS instruction when using GlobalIsel

spec:
https://developer.arm.com/documentation/ddi0602/2022-09/Base-Instructions/ABS--Absolute-value-

Diff Detail

Event Timeline

stuij created this revision.Dec 6 2022, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 5:32 AM
stuij requested review of this revision.Dec 6 2022, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 5:32 AM
paquette added inline comments.Dec 7 2022, 10:28 AM
llvm/test/CodeGen/AArch64/GlobalISel/select-abs.mir
4

The codegen here didn't change at all.

Is it possible to select the scalar abs right now? Should there be a scalar testcase here?

stuij marked an inline comment as done.Dec 16 2022, 9:15 AM
stuij added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/select-abs.mir
4

No, there should not be a scalar testcase here. I tested this at some point and must have decided that CHECK-NEXT is better than NEXT, so why not add it to the patch.

paquette added inline comments.Dec 16 2022, 11:23 AM
llvm/test/CodeGen/AArch64/GlobalISel/select-abs.mir
4

Yeah the update_mir_test_checks script was updated a while back to use CHECK-NEXT. Which is probably better, but introduces a lot of noise to the blamelist when we're updating our tests. :(

I'd recommend not updating this test if you're not actually modifying codegen in this patch. If only because it shows up in the blamelist and makes the commit history a little more confusing.

stuij updated this revision to Diff 483902.Dec 19 2022, 3:59 AM
stuij marked an inline comment as done.

remove changes to select-abs.mir

stuij marked an inline comment as done.Dec 19 2022, 3:59 AM
aemerson added inline comments.Jan 5 2023, 12:52 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
740–750

Can you do the check outside so we can avoid using predicates?

stuij updated this revision to Diff 486589.Jan 5 2023, 8:08 AM

address review comment

stuij marked an inline comment as done.Jan 5 2023, 8:10 AM
aemerson added inline comments.Jan 5 2023, 10:15 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
745–748

Am I reading this wrong or can't this be:

.legalFor({s32, s64})
.legalIf(typeInSet(0, LegalABSVectorTypes)
stuij updated this revision to Diff 486779.Jan 6 2023, 2:05 AM

addressed review comment

stuij marked an inline comment as done.Jan 6 2023, 2:20 AM
stuij added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
745–748

Ah right :) Thanks! I didn't consider multiple predicates for the same purpose could work. Even though almost this exact scenario is used elsewhere in the code, I see now. I should put time into understanding the engine a bit better.

aemerson added inline comments.Jan 6 2023, 2:55 PM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
746–748

Sorry, just spotted something else. This rule now is partially redundant since {s32,s64} are legal. It's just .lowerIf(isScalar(0)), which also means that most of this can be merged with the non-CSSC rules. You can do:

if (HasCSSC)
  ABSActions
    .legalFor({s32, s64})
ABSActions
  .legalFor(PackedVectorAllTypeList)
  .lowerIf(isScalar(0));
stuij updated this revision to Diff 487475.Jan 9 2023, 9:26 AM
stuij marked an inline comment as done.

simplified code some more

stuij marked an inline comment as done.Jan 9 2023, 9:28 AM
stuij added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
745–748

thanks. done!

aemerson accepted this revision.Jan 9 2023, 10:11 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 9 2023, 10:11 AM
This revision was landed with ongoing or failed builds.Jan 10 2023, 3:46 AM
This revision was automatically updated to reflect the committed changes.