This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVEIntrinsicOpts] Convect cntb/h/w/d to vscale intrinsic or constant.
ClosedPublic

Authored by junparser on Jun 24 2021, 5:39 AM.

Diff Detail

Event Timeline

junparser created this revision.Jun 24 2021, 5:39 AM
junparser requested review of this revision.Jun 24 2021, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2021, 5:39 AM
junparser updated this revision to Diff 354228.Jun 24 2021, 5:47 AM

clang-format.

junparser updated this revision to Diff 354427.Jun 24 2021, 8:57 PM

update clang test.

Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2021, 8:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Hi @junparser, the patch looks sensible to me! I just had a couple of minor comments if that's ok.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
658

nit: Do you need to fix the clang-tidy warning here?

662

Could you potentially fold these two cases into one somehow? Maybe with a switch-case statement? I'm just imagining a situation where we might want other patterns too like vl32, vl64, etc.

junparser added inline comments.Jun 28 2021, 4:30 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
662

There is no other special pattern except vl16. But I do think switch-case is more straightforward

junparser updated this revision to Diff 354846.Jun 28 2021, 5:04 AM

Address comments.

david-arm added inline comments.Jun 28 2021, 5:13 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
662

OK, thanks for making this a switch statement. I was just thinking that in the developer manual we say we also support vl1-vl256 so at some point we may add more enums in LLVM too.

671

I was actually wondering if we could commonise this code somehow. Perhaps by setting a MinNumElts variable in the case statements, i.e.

unsigned MinNumElts;
...
case AArch64SVEPredPattern::vl8:
  MinNumElts = Pattern;
  break;
case AArch64SVEPredPattern::vl16:
  MinNumElts = 16;
  break;
}

if (NumElts < MinNumElts) return None;

return Optional<Instruction *>(IC.replaceInstUsesWith(
                             II, ConstantInt::get(II.getType(), NumElts)));
junparser updated this revision to Diff 354857.Jun 28 2021, 5:40 AM

address comments.

david-arm accepted this revision.Jun 30 2021, 9:15 AM

LGTM! Thanks a lot for making the changes. :)

This revision is now accepted and ready to land.Jun 30 2021, 9:15 AM
Matt added a subscriber: Matt.Jun 30 2021, 1:01 PM