Page MenuHomePhabricator

[X86] Avoid SFB - Fix inconsistent codegen with/without debug info
ClosedPublic

Authored by cdawson on May 8 2019, 7:05 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=40969

The functions findPotentiallyBlockedCopies and buildCopy are currently not
accounting for the presence of debug instructions. In the former this results
in the optimization not being trigerred, and in the latter results in
inconsistent codegen.

This patch enables the optimization to be performed in a debug build and
ensures the codegen is consistent with non-debug builds.

Diff Detail

Repository
rL LLVM

Event Timeline

cdawson created this revision.May 8 2019, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 7:05 AM

Hm, can -debugify be run by llc?

lebedev.ri added inline comments.May 8 2019, 7:11 AM
llvm/test/CodeGen/X86/avoid-sfb-g-no-change.ll
1 ↗(On Diff #198651)

Use utils/update_llc_test_checks

Hm, can -debugify be run by llc?

No. The debugify pass itself lives inside llvm/tools/opt.

cdawson marked an inline comment as done.May 8 2019, 7:22 AM
cdawson added inline comments.
llvm/test/CodeGen/X86/avoid-sfb-g-no-change.ll
1 ↗(On Diff #198651)

I tried that initially but it doesn't generate any checks for the IR.

The point is not that a certain set of moves comes out, but that the *same* set of moves comes out regardless of debug info.
So, the test wants to have two copies of the same function, one with debug instructions and one without; and both functions should produce the same sequence of moves.

To keep the test from being really tedious, I am pretty sure you can run FileCheck twice and have each run look at a different function, with clever use of -D on the FileCheck command line. This probably means you have to capture the llc output in a file, but that's okay.

aprantl added inline comments.May 8 2019, 9:40 AM
llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
410 ↗(On Diff #198651)

std::prev ?

I wonder if this should be an MIR test? Fewer moving parts.

cdawson updated this revision to Diff 198842.May 9 2019, 8:47 AM

Addressed review comments

cdawson marked an inline comment as done.May 9 2019, 8:48 AM
probinson added inline comments.May 9 2019, 10:06 AM
llvm/test/CodeGen/X86/avoid-sfb-g-no-change.mir
247 ↗(On Diff #198842)

Add ; DEBUG-LABEL: name: nodebug at the end, to prove the CHECK lines are all looking at the correct copy of the function.

cdawson updated this revision to Diff 199001.May 10 2019, 4:30 AM

Addressed revision comments

cdawson marked an inline comment as done.May 10 2019, 4:31 AM
This revision is now accepted and ready to land.May 10 2019, 4:53 AM
This revision was automatically updated to reflect the committed changes.

Hi, it seems that this patch results in an assertion error when building zircon:

clang: /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-2/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:293: int getAddrOffset(llvm::MachineInstr *): Assertion `AddrOffset != -1 && "Expected Memory Operand"' failed.

Could you take a look and either fix or revert this patch? I filed a bug report with the full stack trace and reproducer you could use (https://bugs.llvm.org/show_bug.cgi?id=41838)

cdawson updated this revision to Diff 199624.May 15 2019, 8:52 AM

This fix addresses https://bugs.llvm.org/show_bug.cgi?id=41838

The previous test case didn't trigger the codepath which contained the assertion. The updated test rectifies this.

cdawson reopened this revision.May 15 2019, 8:53 AM

Reopening after updating patch

This revision is now accepted and ready to land.May 15 2019, 8:53 AM
cdawson updated this revision to Diff 199627.May 15 2019, 9:19 AM

Upload full diff with both sets of changes.

cdawson requested review of this revision.May 15 2019, 9:20 AM

Requesting review

aprantl accepted this revision.Wed, May 22, 9:24 AM
This revision is now accepted and ready to land.Wed, May 22, 9:24 AM
This revision was automatically updated to reflect the committed changes.