This is an archive of the discontinued LLVM Phabricator instance.

[X86] merge "={eax}" and "~{eax}" into "=&eax" for MSInlineASM
ClosedPublic

Authored by FreddyYe on Jan 11 2021, 9:10 PM.

Details

Summary

This patch is to fix an end-to-end bug (https://gcc.godbolt.org/z/GG44f6) on recent rewrite on back-end's RegAllocFast.cpp. I created a fix patch in back-end before (https://reviews.llvm.org/D93983). Both solutions are reasonable to me. The proof of this patch is that GNU inline ASM doesn't support setting one register as both "output" and "clobber"(Both clang and gcc enable this feature https://gcc.godbolt.org/z/4n6W56). Moreover, since MS inline ASM didn't define semantics of "output" or "clobber". So I think LLVM IR should follow this rule with GNU inline ASM, too.

Diff Detail

Event Timeline

FreddyYe requested review of this revision.Jan 11 2021, 9:10 PM
FreddyYe created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 9:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
FreddyYe edited the summary of this revision. (Show Details)Jan 11 2021, 9:25 PM
pengfei added inline comments.Jan 11 2021, 9:54 PM
clang/lib/CodeGen/CGStmt.cpp
2488

={eax} is set in X86_32TargetCodeGenInfo::addReturnRegisterOutputs, which also handles EAX:EDX case by =A. I think it's better handle it in the function to cover the other case as well.

FreddyYe added inline comments.Jan 11 2021, 11:27 PM
clang/lib/CodeGen/CGStmt.cpp
2488

THX for review. Agree. I'll update.

FreddyYe updated this revision to Diff 316324.Jan 12 2021, 10:37 PM

consider "=A" into the patch

pengfei added inline comments.Jan 12 2021, 11:27 PM
clang/lib/CodeGen/CGStmt.cpp
2493

Should consider the Clobber == "edx"?

FreddyYe updated this revision to Diff 318123.Jan 20 2021, 11:56 PM

consider Clobber == "edx"

FreddyYe updated this revision to Diff 318128.Jan 21 2021, 12:43 AM

Fix Lint: Pre-merge checks

pengfei added inline comments.Jan 26 2021, 2:15 AM
clang/lib/CodeGen/CGStmt.cpp
2490

If Clobber is edx only, we shouldn't change "={eax}" to "=&{eax}".

FreddyYe added inline comments.Jan 26 2021, 3:29 AM
clang/lib/CodeGen/CGStmt.cpp
2490

Yes! I'll update and add a test. THX for review!

FreddyYe updated this revision to Diff 319282.Jan 26 2021, 6:22 AM

If Clobber is edx only, don't change "={eax}" to "=&{eax}".

FreddyYe updated this revision to Diff 319284.Jan 26 2021, 6:26 AM

refine clang-format

pengfei accepted this revision.Jan 26 2021, 6:25 PM

LGTM.

clang/lib/CodeGen/CGStmt.cpp
2491

I'm not sure if there's real case that there's "={edx}" for MS inline asm, but there's no problem we handle it here.

This revision is now accepted and ready to land.Jan 26 2021, 6:25 PM