This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] NFC: tidy up isel lowering
ClosedPublic

Authored by c-rhodes on Jan 30 2022, 4:37 AM.

Diff Detail

Event Timeline

c-rhodes created this revision.Jan 30 2022, 4:37 AM
c-rhodes requested review of this revision.Jan 30 2022, 4:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2022, 4:37 AM
Matt added a subscriber: Matt.Jan 31 2022, 8:05 AM

Hi @c-rhodes, could you add a description of why this refactoring is needed please? It's just not really obvious to me at least why we're shuffling code around.

Hi @c-rhodes, could you add a description of why this refactoring is needed please? It's just not really obvious to me at least why we're shuffling code around.

Hi @david-arm, whilst adding legal types <-> register classes for Streaming SVE (D118561) I noticed that hasSVE predication block set operation actions for opcodes that may not be legal in Streaming SVE, and given there's another hasSVE further down that sets actions with loops over the same types I thought it made more sense to move them there.

Hi @c-rhodes, thanks for the explanation! Could you add something like that to the commit message please? Then I'm happy to accept!

sdesmalen accepted this revision.Feb 1 2022, 8:42 AM

I agree with @david-arm that the commit message would benefit from some extra words. Otherwise, LGTM!

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1179–1182

nit: I thought the default was Legal, so maybe these 4 lines can be removed?

This revision is now accepted and ready to land.Feb 1 2022, 8:42 AM
c-rhodes added inline comments.Feb 2 2022, 12:27 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1179–1182

nit: I thought the default was Legal, so maybe these 4 lines can be removed?

That crossed my mind as well, but these opcodes default to expand (set in TargetLoweringBase). A warning in setOperationAction when marking an operation that is legal by default legal might be nice, or even when setting it to the same operation, but I suspect there will be many such occurrences.

This revision was landed with ongoing or failed builds.Feb 2 2022, 1:03 AM
This revision was automatically updated to reflect the committed changes.