This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

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

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

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

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.May 22 2019, 9:24 AM
This revision is now accepted and ready to land.May 22 2019, 9:24 AM
This revision was automatically updated to reflect the committed changes.