This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add support for boolean logic and fcmp.
ClosedPublic

Authored by efriedma on Mar 26 2020, 7:56 PM.

Details

Summary

These kind of go together, since fcmp expansion generates boolean operations.

Diff Detail

Event Timeline

efriedma created this revision.Mar 26 2020, 7:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 7:56 PM
sdesmalen added inline comments.Mar 27 2020, 2:44 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7563

It seems like this code is missing a guard for VT.isScalableVector() (same for the aarch64_sve_whilelo case below)

llvm/lib/Target/AArch64/SVEInstrFormats.td
1374

Do we want to create a SVE_2_Op_<..>_Pat or something for these? There will be other instructions where this would be useful as well.

nit: please reverse the order so it matches the patterns straight above it.

efriedma marked 2 inline comments as done.Mar 27 2020, 12:37 PM
efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7563

Not sure what you think needs to be handled; non-scalable i1 vectors aren't legal.

llvm/lib/Target/AArch64/SVEInstrFormats.td
1374

I'm not sure what that would look like. Are you proposing something like SVE_2_Op_PTRUE_H_Pat/SVE_2_Op_PTRUE_S_Pat/etc.?

cameron.mcinally added inline comments.
llvm/lib/Target/AArch64/SVEInstrFormats.td
1374

There's something similar in D71712:

class SVE_2_Op_AllActive_Pat<ValueType vtd, SDPatternOperator op, ValueType vt1,
                             ValueType vt2, Instruction inst, Instruction ptrue>
: Pat<(vtd (op vt1:$Op1, vt2:$Op2)),
      (inst (ptrue 31), $Op1, $Op2)>;

Seems like a good fit, but I'm not sure...

efriedma marked an inline comment as done.Mar 27 2020, 3:41 PM
efriedma added inline comments.
llvm/lib/Target/AArch64/SVEInstrFormats.td
1374

So this would be written like the following?

def : SVE_2_Op_AllActive_Pat<nxv2i1, op_nopred, nxv2i1, nxv2i1,
                             !cast<Instruction>(NAME), PTRUE_D>;

It doesn't seem much better than what I've written, but okay, I guess.

efriedma updated this revision to Diff 253723.Mar 30 2020, 3:39 PM

Address review comments

sdesmalen accepted this revision.Mar 31 2020, 10:51 AM

LGTM!

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

Yes, of course! Thanks for adding that comment.

llvm/lib/Target/AArch64/SVEInstrFormats.td
1374

Cheers, FWIW this makes it a bit more readable to me (especially in conjunction with the use of SVE_3_Op_Pat above).

1376

This file doesn't seem too strict on the 80char limit, so I think having this on the same line would make it more readable.

This revision is now accepted and ready to land.Mar 31 2020, 10:51 AM
efriedma marked an inline comment as done.Mar 31 2020, 11:44 AM
efriedma added inline comments.
llvm/lib/Target/AArch64/SVEInstrFormats.td
1376

It's not strict, yes, but I think going over 100 columns is probably not a good idea.

This revision was automatically updated to reflect the committed changes.