This is an archive of the discontinued LLVM Phabricator instance.

Differential D148817

[RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.
ClosedPublic

Authored by craig.topper on Apr 20 2023, 10:25 AM.

Details

Reviewers
kito-cheng
asb
jrtc27
reames
Commits
rGea0dae096189: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an…
Summary

Can be disabled with -mno-default-build-attributes just like ARM.

Diff Detail

Event Timeline

craig.topper created this revision.Apr 20 2023, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 10:25 AM
Herald added subscribers: jobnoorman, luke, VincentWu and 27 others. · View Herald Transcript
craig.topper requested review of this revision.Apr 20 2023, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 10:25 AM
Herald added subscribers: • pcwang-thead, eopXD, MaskRay. · View Herald Transcript
jrtc27 added a comment.Apr 20 2023, 10:27 AM

Surprised that we didn't already do this; seems like a bit of an oversight.

clang/test/Driver/riscv-default-build-attributes.s
2

Do we use /// for non-lit/FileCheck lines like we use ;; in IR tests?

12

Gap is a bit odd

13

missing "for"

craig.topper added inline comments.Apr 20 2023, 11:18 AM
clang/test/Driver/riscv-default-build-attributes.s
2

I don't know. I copied it from ARM's test.

Harbormaster completed remote builds in B226911: Diff 515382.Apr 20 2023, 12:10 PM
craig.topper updated this revision to Diff 515484.Apr 20 2023, 2:22 PM

Address review comments

jrtc27 added a comment.Apr 20 2023, 2:30 PM

Some more... hopefully spotted everything this time, sorry

clang/test/Driver/riscv-default-build-attributes.s
2
4

= for these

4

Also aren't these indented 1 too many spaces?

21

There aren't any uses of -verify?

craig.topper added inline comments.Apr 20 2023, 3:06 PM
clang/test/Driver/riscv-default-build-attributes.s
4

Cleary I should not have copied ARM's test :(

jrtc27 added inline comments.Apr 20 2023, 3:11 PM
clang/test/Driver/riscv-default-build-attributes.s
4

Yeah... :( ARM and AArch64 tests can be pretty iffy in my experience

craig.topper updated this revision to Diff 515511.Apr 20 2023, 3:32 PM

Fix more review comments

Harbormaster completed remote builds in B227000: Diff 515511.Apr 20 2023, 7:38 PM
asb accepted this revision.Apr 21 2023, 10:27 AM

I'd appreciate some riscv32 RUN lines for completeness, but otherwise LGTM.

This revision is now accepted and ready to land.Apr 21 2023, 10:27 AM
This revision was landed with ongoing or failed builds.Apr 21 2023, 10:54 AM
Closed by commit rGea0dae096189: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an… (authored by craig.topper). · Explain Why
This revision was automatically updated to reflect the committed changes.
craig.topper added a commit: rGea0dae096189: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an….

Revision Contents

PathSize
clang/
lib/
Driver/
ToolChains/
6 lines
test/
Driver/
29 lines

Diff 515832

clang/lib/Driver/ToolChains/Clang.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 7,897 Lines • ▼ Show 20 Lines
void ClangAs::AddRISCVTargetArgs(const ArgList &Args, void ClangAs::AddRISCVTargetArgs(const ArgList &Args,
ArgStringList &CmdArgs) const { ArgStringList &CmdArgs) const {
const llvm::Triple &Triple = getToolChain().getTriple(); const llvm::Triple &Triple = getToolChain().getTriple();
StringRef ABIName = riscv::getRISCVABI(Args, Triple); StringRef ABIName = riscv::getRISCVABI(Args, Triple);
CmdArgs.push_back("-target-abi"); CmdArgs.push_back("-target-abi");
CmdArgs.push_back(ABIName.data()); CmdArgs.push_back(ABIName.data());
if (Args.hasFlag(options::OPT_mdefault_build_attributes,
options::OPT_mno_default_build_attributes, true)) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-riscv-add-build-attributes");
}
} }
void ClangAs::ConstructJob(Compilation &C, const JobAction &JA, void ClangAs::ConstructJob(Compilation &C, const JobAction &JA,
const InputInfo &Output, const InputInfoList &Inputs, const InputInfo &Output, const InputInfoList &Inputs,
const ArgList &Args, const ArgList &Args,
const char *LinkingOutput) const { const char *LinkingOutput) const {
ArgStringList CmdArgs; ArgStringList CmdArgs;
▲ Show 20 Lines • Show All 607 Lines • Show Last 20 Lines

clang/test/Driver/riscv-default-build-attributes.s

  • This file was added.
//// Enabled by default for assembly
// RUN: %clang --target=riscv32 -### %s 2>&1 \
jrtc27Unsubmitted
Not Done
ReplyInline Actions

Do we use /// for non-lit/FileCheck lines like we use ;; in IR tests?

jrtc27: Do we use /// for non-lit/FileCheck lines like we use ;; in IR tests?
craig.topperAuthorUnsubmitted
Done
ReplyInline Actions

I don't know. I copied it from ARM's test.

craig.topper: I don't know. I copied it from ARM's test.
jrtc27Unsubmitted
Not Done
ReplyInline Actions
- //// Enabled by default for assembly
+ /// Enabled by default for assembly
// RUN: %clang --target=riscv64 -### %s 2>&1 \
jrtc27:
// RUN: | FileCheck %s --check-prefix=CHECK-ENABLED
// RUN: %clang --target=riscv64 -### %s 2>&1 \
jrtc27Unsubmitted
Not Done
ReplyInline Actions

= for these

jrtc27: `=` for these
jrtc27Unsubmitted
Not Done
ReplyInline Actions

Also aren't these indented 1 too many spaces?

jrtc27: Also aren't these indented 1 too many spaces?
craig.topperAuthorUnsubmitted
Done
ReplyInline Actions

Cleary I should not have copied ARM's test :(

craig.topper: Cleary I should not have copied ARM's test :(
jrtc27Unsubmitted
Not Done
ReplyInline Actions

Yeah... :( ARM and AArch64 tests can be pretty iffy in my experience

jrtc27: Yeah... :( ARM and AArch64 tests can be pretty iffy in my experience
// RUN: | FileCheck %s --check-prefix=CHECK-ENABLED
/// Can be forced on or off for assembly.
// RUN: %clang --target=riscv32 -### %s 2>&1 -mno-default-build-attributes \
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// RUN: %clang --target=riscv64 -### %s 2>&1 -mno-default-build-attributes \
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// RUN: %clang --target=riscv32 -### %s 2>&1 -mdefault-build-attributes \
jrtc27Unsubmitted
Not Done
ReplyInline Actions

Gap is a bit odd

jrtc27: Gap is a bit odd
// RUN: | FileCheck %s --check-prefix=CHECK-ENABLED
jrtc27Unsubmitted
Not Done
ReplyInline Actions

missing "for"

jrtc27: missing "for"
// RUN: %clang --target=riscv64 -### %s 2>&1 -mdefault-build-attributes \
// RUN: | FileCheck %s --check-prefix=CHECK-ENABLED
/// Option ignored for C/C++ (since we always emit hardware and ABI build
/// attributes during codegen).
// RUN: %clang --target=riscv32 -### -x c %s -mdefault-build-attributes 2>&1 \
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// RUN: %clang --target=riscv64 -### -x c %s -mdefault-build-attributes 2>&1 \
jrtc27Unsubmitted
Not Done
ReplyInline Actions

There aren't any uses of -verify?

jrtc27: There aren't any uses of -verify?
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// RUN: %clang --target=riscv32 -### -x c++ %s -mdefault-build-attributes 2>&1 \
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// RUN: %clang --target=riscv64 -### -x c++ %s -mdefault-build-attributes 2>&1 \
// RUN: | FileCheck %s --check-prefix=CHECK-DISABLED
// CHECK-DISABLED-NOT: "-riscv-add-build-attributes"
// CHECK-ENABLED: "-riscv-add-build-attributes"