This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add function that check extension name with version
ClosedPublic

Authored by BeMg on Jun 7 2023, 11:08 PM.

Details

Summary

Check whether a extension string with version is valid, and get the targetfeature from it.

New functions be used in RISCVISAInfo for https://reviews.llvm.org/D151730.

Diff Detail

Event Timeline

BeMg created this revision.Jun 7 2023, 11:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 11:08 PM
BeMg requested review of this revision.Jun 7 2023, 11:08 PM
BeMg edited the summary of this revision. (Show Details)Jun 7 2023, 11:12 PM
BeMg updated this revision to Diff 529509.Jun 7 2023, 11:13 PM
BeMg edited the summary of this revision. (Show Details)

update

BeMg retitled this revision from [RISCV] check extension name with version to [RISCV] Add function that check extension name with version.

Could you add some testcase in llvm/unittests/Support/RISCVISAInfoTest.cpp?

craig.topper added inline comments.Jun 8 2023, 11:39 AM
llvm/lib/Support/RISCVISAInfo.cpp
1261

This function is poorly named. Shouldn't it be findLastNonVersionCharacter?

1262

I think writing as an assignment is more common

StringRef Name = Ext.substr(0, Pos);
StringRef Vers = Ext.substr(Pos);
craig.topper added inline comments.Jun 8 2023, 11:40 AM
llvm/lib/Support/RISCVISAInfo.cpp
1261

Or it should be findFirstVersionCharacter and take care of the +1

BeMg updated this revision to Diff 529817.Jun 8 2023, 9:18 PM
  1. update function body
  2. Add unit test
  3. findFirstNonVersionCharacter -> findLastNonVersionCharacter
craig.topper added inline comments.Jun 8 2023, 9:41 PM
llvm/lib/Support/RISCVISAInfo.cpp
223

Can you make a separate patch for renaming this?

BeMg added inline comments.Jun 8 2023, 9:54 PM
llvm/lib/Support/RISCVISAInfo.cpp
223

OK

BeMg updated this revision to Diff 529862.Jun 9 2023, 2:07 AM

rename findLastNonVersionCharacter with https://reviews.llvm.org/D152506

craig.topper added inline comments.Jun 23 2023, 5:52 PM
llvm/lib/Support/RISCVISAInfo.cpp
1283

I don't think Vers needs to exist. It's only used for !Vers.empty() which is equivalent to Pos != Ext.size()

llvm/unittests/Support/RISCVISAInfoTest.cpp
517

Use EXPECT_TRUE and EXPECT_FALSE

BeMg updated this revision to Diff 534419.Jun 25 2023, 9:06 PM
  1. Remove Vers and use Pos != Ext.size()
  2. Update testcase by using EXPECT_TRUE|EXPECT_FALSE
BeMg updated this revision to Diff 535628.Jun 28 2023, 10:43 PM
  1. Let isSupportedExtension support extension name with version
  2. Remove isSupportedExtensionWithVersion
BeMg updated this revision to Diff 535647.Jun 28 2023, 11:54 PM

Revert previous update

BeMg added a comment.Jun 28 2023, 11:55 PM

And rename getTargetFeatureFromOneExt into getTargetFeatureForExtension

BeMg updated this revision to Diff 549888.Aug 14 2023, 5:22 AM

rebase and update testcase

BeMg updated this revision to Diff 549892.Aug 14 2023, 5:46 AM

rebase and update testcase

Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 5:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
BeMg updated this revision to Diff 550238.Aug 15 2023, 3:29 AM

revert last patch because it's wrong one

This revision is now accepted and ready to land.Aug 15 2023, 6:58 PM
This revision was landed with ongoing or failed builds.Aug 20 2023, 9:07 PM
This revision was automatically updated to reflect the committed changes.