Page MenuHomePhabricator

[COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg
ClosedPublic

Authored by glandium on Feb 1 2019, 7:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

glandium created this revision.Feb 1 2019, 7:59 PM
smeenai added subscribers: hans, smeenai.

+@hans for the release branch.

Missing testcase changes. (It should be possible to check that we aren't inserting incorrect truncation/extension operations in the IR.)

Thanks for finding out and fixing this. Seems there is also issue in expanding _WriteStatusReg in CodeGenFunction::EmitAArch64BuiltinExpr. The last argument for _WriteStatusReg is zero extended to __in64, which is not expected (see below link).

https://github.com/llvm-mirror/clang/blob/8070ca12f87e66f76db528c107e9d291f4a91498/lib/CodeGen/CGBuiltin.cpp#L7100

Yes, we should fix CodeGenFunction::EmitAArch64BuiltinExpr to eliminated the unnecessary calls to CreateZext/CreateTrunc. (With this patch, they're no-ops, but better to clean up the code.)

(It should be possible to check that we aren't inserting incorrect truncation/extension operations in the IR.)

I don't know how to do that.

In test/CodeGen/arm64-microsoft-status-reg.cpp, you can write something like // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD2:.*]]), then // CHECK-IR-NEXT: store i64 %[[VAR]] on the next line. See also http://llvm.org/docs/CommandGuide/FileCheck.html .

glandium updated this revision to Diff 185218.Feb 4 2019, 7:01 PM
glandium edited the summary of this revision. (Show Details)

Updated EmitAArch64BuiltinExpr per https://reviews.llvm.org/D57636#1383751 and the testcase per https://reviews.llvm.org/D57636#1384348

@efriedma can you take another look? Ideally, this should be backported to the release_80 branch, so that would need to be landed asap.

efriedma accepted this revision.Feb 7 2019, 4:56 PM

LGTM. Do you want me to commit this for you?

This revision is now accepted and ready to land.Feb 7 2019, 4:56 PM

LGTM. Do you want me to commit this for you?

Yes, please. Thank you.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 5:17 PM