Page MenuHomePhabricator
Differential D93789

Add accessors for MCSubtargetInfo CPU and Feature tables
AcceptedPublic
Actions

Authored by jrmuizel on Wed, Dec 23, 6:10 PM.

Details

Reviewers
grosbach
nikic
craig.topper
Summary

This is needed for -C target-cpu=help and -C target-feature=help in rustc

Diff Detail

Unit TestsFailed
View All

TimeTest
15,410 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm
70 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

jrmuizel created this revision.Wed, Dec 23, 6:10 PM
Herald added a subscriber: JDevlieghere. · View Herald TranscriptWed, Dec 23, 6:10 PM
jrmuizel requested review of this revision.Wed, Dec 23, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Dec 23, 6:10 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
jrmuizel added a reviewer: grosbach.Wed, Dec 23, 6:12 PM

This comes from https://github.com/rust-lang/llvm-project/commit/015eb777cf3b1890083df90d281f504d34398815

Harbormaster completed remote builds in B83444: Diff 313638.Wed, Dec 23, 6:57 PM
jrmuizel added a reviewer: nikic.Tue, Jan 12, 7:52 AM
nikic added a reviewer: craig.topper.Tue, Jan 12, 9:34 AM
nikic accepted this revision.Sun, Jan 17, 1:51 AM

LGTM. I'm not particularly familiar with this code, but don't see an issue with exposing this information. Currently MCSubtargetInfo directly incorporates the logic for printing a help text to the error stream, which is very dubious design. It should be possible to obtain the same information for printing a help text manually.

This revision is now accepted and ready to land.Sun, Jan 17, 1:51 AM
jrmuizel added a comment.Sun, Jan 17, 6:08 AM

@nikic can you land this?

fhahn added a subscriber: fhahn.Sun, Jan 17, 6:28 AM
fhahn added inline comments.
llvm/include/llvm/MC/MCSubtargetInfo.h
229

Can you add doc-comments for the new functions?

jrmuizel updated this revision to Diff 317217.Sun, Jan 17, 6:56 AM

Add comments

Harbormaster completed remote builds in B85518: Diff 317217.Sun, Jan 17, 8:17 AM
craig.topper added a comment.Sun, Jan 17, 9:17 AM

I’m not against this, but do want to mention the help text is incorrect for x86-64. It lists CPUs that don’t support 64-bit. Using one of those CPUs will trigger a fatal error. A better list is fillValidCPUList from X86TargetParser.cpp in lib/Support. It takes a mode argument. That’s the interface clang uses.

jrmuizel added a comment.Sun, Jan 17, 5:02 PM

I filed https://github.com/rust-lang/rust/issues/81148 about improving the list the Rust uses.

jrmuizel updated this revision to Diff 317250.Sun, Jan 17, 5:04 PM

Update the doc comments to be less wrong.

Harbormaster completed remote builds in B85544: Diff 317250.Sun, Jan 17, 5:42 PM
nikic added a comment.Mon, Jan 18, 11:28 AM
In D93789#2503709, @craig.topper wrote:

I’m not against this, but do want to mention the help text is incorrect for x86-64. It lists CPUs that don’t support 64-bit. Using one of those CPUs will trigger a fatal error. A better list is fillValidCPUList from X86TargetParser.cpp in lib/Support. It takes a mode argument. That’s the interface clang uses.

The fillValidCPUArchList() interface looks pretty annoying in that it's non-virtual. One needs to call different helper functions for each target.

Revision Contents
Changeset List

PathSize
llvm/
include/
llvm/
MC/
6 lines

Diff 317250

View Options

llvm/include/llvm/MC/MCSubtargetInfo.h

Loading...