Page MenuHomePhabricator

[Codegen] skip debug instr to avoid code change
AcceptedPublic

Authored by yechunliang on Aug 20 2019, 3:54 AM.

Details

Summary

Skip debugging instruction and CFI_INSTRUCTION to avoid different codegen with/without -g by branch-folder pass. Most of ComputeCommonTailLength uses the "countsAsInstruction" helper to skip over debug instructions, but the last two loops don't.
Add branch-folder-with-debug.mir test

the bug is reported in
https://bugs.llvm.org/show_bug.cgi?id=42138

Diff Detail

Event Timeline

yechunliang created this revision.Aug 20 2019, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 3:54 AM

Indeed, this'll need a MIR test for the BranchFolding pass to check for future regressions, and to demonstrate that this patch fixes the bug report. There are some MIR test examples in the llvm/test/CodeGen/MIR/X86 directory.

You should also take a quick look at the history of the file to try and find other reviewers. While Greg and I know some debug things, ideally someone who knows more about the branch folding pass should look at this too, in case there's a more general problem we're not aware of. People who've recently made significant changes to the file are good candidates (or they might know who else should review this).

yechunliang added a comment.EditedAug 21 2019, 2:09 AM

thanks for review and comments. While adding test code, found another error about "unnamed alloca" before branch-folder by llc, I'll investigate the issue before submit the test code.

Please allow me to add Hsiangkai and Petar as reviewer, as you had contributed related patches about debug and CFI instruction skipping. Thanks.

add branch-folder-with-debug.mir test

@yechunliang Thanks for the test case. Have you tried to simplify it?

llvm/test/CodeGen/MIR/X86/branch-folder-with-debug.mir
157

I think we can delete the attributes.

238

We can attach more instructions to the same !DILocation and by doing that we are simplifying the test case.

275

I think we don't need most of the MIR Function attributes, so we can delete some of them.

dstenb added a subscriber: dstenb.Aug 26 2019, 4:58 AM

It might be worthwhile to try to reduce the reproducer with Bugpoint.

Perhaps there exists some neat wrapper/template for bugpointing these types of bugs, but I personally just pass a custom script to Bugpoint of the following form:

opt $1 -S | llc [...] -o with_debug.o
opt $1 -S -strip-debug | llc [...] -o without_debug.o
[exit 1 if text sections differ]
exit 0

If you have not used Bugpoint with custom scripts before, then I think the following blog post can be helpful: http://blog.llvm.org/2015/11/reduce-your-testcases-with-bugpoint-and.html

Simplify MIR test case

@djtodoro thanks for review, followed your comments and guide from https://llvm.org/docs/MIRLangRef.html#id14, I have simplified mir test code by removing all unnecessary code for FileCheck.
"llc -o - test.mir -mtriple=x86_64-- -run-pass=branch-folder | FileCheck test.mir"

@dstenb thanks for guidelines, I know bugpoint but have not used yet, this should be a useful tool and I'll try it.

jmorse added a comment.Sep 2 2019, 8:56 AM

Looking good; a couple of extra nits, then it's ready to go.

I think the CHECK lines need strengthening a little, by changing for example:

; CHECK: bb.5:
; CHECK: successors: %bb.6(0x80000000)

to

; CHECK: bb.5:
; CHECK-NEXT: successors: %bb.6(0x80000000)

Otherwise, FileCheck could in theory accept any number of lines between "bb.5:" and the successor list, including other blocks. Due to the way that the test is written that probably wouldn't be a problem right now, but tests get updated all the time, and explicitly connecting pairs of CHECK lines with -NEXT will make it harder for a future developer to make a mistake when editing this test.

IMO it'd be best to replace the "0x80000000" ... etc constants in the successors list with a regex (i.e. "{{0x[0-9]+}}") to avoid testing too much. These are branch frequency weights that aren't important for this test.

Ideally there would also be a comment in the test file explaining what should happen -- i.e. "Branch folder should ignore the DBG_VALUE between [...] and merge these blocks", or whatever it's supposed to be doing.

llvm/test/CodeGen/MIR/X86/branch-folder-with-debug.mir
46

NB, I believe the convention is to put the RUN line at the very top of the file -- I don't think it's rule, but it's best to avoid surprise.

58

If this successor list is going to be tested for, the block it's in should be tested too. Also on line 64.

yechunliang edited the summary of this revision. (Show Details)

update branch-folder-with-debug.mir follow the review comments

  • update CHECK block lines before successors check
  • Add "CHECK-NEXT" for successors after block
  • Simplify address after successors block
  • move RUN line to top and add more descriptions.
jmorse accepted this revision.Sep 3 2019, 2:06 AM

LGTM, thanks for digging into this!

This revision is now accepted and ready to land.Sep 3 2019, 2:06 AM