Branch folding causes different code generation at "-O2 -g" and "-O2"

[kromanova]

Summary

This is a fix for PR# 19051. I noticed code gen differences due to code motion when running tests with and without the debug info at O2.

There is a problem in branch folding. The purpose of the following loop seems to be to skip the debug info

while (PI != MBB->begin() && Loc->isDebugValue())

but it doesn't actually do that. If Loc is not a DebugValue the loop does nothing, otherwise it iterates to the beginning of the block.

Here is a fix that does skip the debug info.

while (PI != MBB->begin() && PI->isDebugValue())

The testcase test/CodeGen/X86/dbg-changes-codegen-branch-folding.ll is checking that the same instruction sequence generated with and without the debug info.

[kromanova]

I will change BZ #19051 into PR #19051 before I commit.

[chandlerc]

Looks good. Generally fine to commit unless you find an existing test case that this should just be added to.

test/CodeGen/X86/dbg-changes-codegen-branch-folding.ll

4

We typically use a link rather than "BZ": http://llvm.org/PR19051

Are there really no other tests for debug info changing codegen in the x86 tree?

15–17

I don't think we need a full debug-info test case here.

Just put some stub debug_value calls into the test in the appropriate places with null metadata. It should still work just as well and won't require the full Clang-generated DWARF-structured debug info metadata.

[probinson]

Are there really no other tests for debug info changing codegen in the
x86 tree?

The LLVM test infrastructure doesn't support doing that in a general way.
We've been finding these things in a test suite where we can make the
test driver compile the same file two ways and compare the generated code.
It's easy to do that when you start with C/C++ and just add -g but not
so easy when you're starting with IR.

Personally I think inserting IR instructions to carry debug annotations
is a poor choice, but replacing it with something that optimizations
are less inclined to screw up would be a really big project.
--paulr

[echristo]

I wouldn't worry about it, just that code generation is often wildly different on atom and so tests can fail pretty quickly on the atom bots. It was mostly a heads up to watch the bots after committing something that looks at particular code generation choices.

[kromanova]

• it has been built and tested against trunk@203176 (Mar6)
• debug info has been reduced a lot (thanks to Trevor's script that strips away unneeded debug info). Generating null metadata with stub debug_value calls resulted in an assertion, so I kept minimal metadata (7 entries only).
• BZ#19051 was replaced with http://llvm.org/PR19051
• the test case was checked with -mcpu=atom

Let me know if more corrections are needed or if it's OK to commit.

[kromanova]

Hi Chandler,
This bugfix is still sitting in my queue. Is it OK to commit? If you are busy, maybe Eric could review?

I will make a small change to the testcase before I commit. I will add -mtriple-x86_64-linux to the RUN line.

Thank you!
Katya.

[kromanova]

Hi Chandler,

I think I have shortened the metadata as much as I could. Out of 91 entries that I had in the original patch, I now have only 7.

!38 = metadata !{i32 786688, null, metadata !"var2", null, i32 20, null, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [var2] [line 20]
!48 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
!49 = metadata !{i32 1, metadata !"Debug Info Version", i32 1}
!50 = metadata !{metadata !"clang version 3.5 (202418)"}
!60 = metadata !{i32 786689, null, metadata !"this", null, i32 16777216, null, i32 1088, null} ; [ DW_TAG_arg_variable ] [this] [line 0]
!62 = metadata !{i8* getelementptr inbounds ([1 x i8]* @.str, i64 0, i64 0)}
!63 = metadata !{i32 786689, null, metadata !"value", null, i32 33554439, null, i32 0, null} ; [ DW_TAG_arg_variable ] [value] [line 7]

If I understood you correctly, I should replace some of these entries with “metadata !{null}”. Is this what you meant?

The compiler is asserting if I substitute
!63 = metadata !{i32 786689, null, metadata !"value", null, i32 33554439, null, i32 0, null} ; [ DW_TAG_arg_variable ] [value] [line 7]
with
!63 = metadata !{null} ;

The test started to pass (i.e. the same code generated at –O2 –g vs -O2), after I substituted
!60 = metadata !{i32 786689, null, metadata !"this", null, i32 16777216, null, i32 1088, null} ; [ DW_TAG_arg_variable ] [this] [line 0]
with
!60 = metadata !{null} ;

Replacing any of the 7 metadata entries with metadata !{null} ; either caused the test to produce the same code at –O2 –g/-O2 or caused the assertion failure. Please let me know if you meant to reduce the test in some other way.

Thanks!
Katya.

From: [e-mail] [mailto:[e-mail]] On Behalf Of Chandler Carruth
Sent: Thursday, March 13, 2014 5:02 PM
To: [e-mail]
Cc: Chandler Carruth; Romanova, Katya; Commit Messages and Patches for LLVM
Subject: Re: [PATCH] Branch folding causes different code generation at "-O2 -g" and "-O2"

[kromanova]

Hi Chandler,

I have posted my previous comment and didn't hear back from you for a while. Sorry to ping you again. I think I have shortened the metadata as much as I could. Out of 91 metadata entries that I had in the original patch, I have only 7 metadata entries now.

I wasn't able to shorten the testcase more by replacing
!<n> = metadata !{blah, blah, blah, ..., blah} ; with
!<n> = metadata !{null} ;
because either the test stops to reproduce the original bug or it asserts. If you meant to shorten metadata differently, please give me a specific example of what you had in mind or point me to the testcase that is doing a similar thing.

Thanks!
Katya.

P.S. Out of curiosity, I checked how many metadata entries other debug-info related tests in llvm/etst/CodeGen/X86 have. This amount ranged from 12 to 72 entries. Is there a specific reason why having only 7 metadata entries in my test is not acceptable?

[chandlerc]

Gah.

Fully stubbing out the debug info doesn't work because we do a bunch of other optimization passes in llc when we only want to test one thing. :: sigh ::

Anyways, LGTM.

[Eugene.Zelenko]

Committed in rL204865.

lib/CodeGen/BranchFolding.cpp

   // branch from condition setting instruction.
   MachineBasicBlock::iterator PI = Loc;
   --PI;
-  while (PI != MBB->begin() && Loc->isDebugValue())
+  while (PI != MBB->begin() && PI->isDebugValue())
     --PI;
 
   bool IsDef = false;

test/CodeGen/X86/dbg-changes-codegen-branch-folding.ll

+; RUN: llc -march=x86-64 < %s | FileCheck %s
+; RUN: opt -strip-debug < %s | llc -march=x86-64 | FileCheck %s
+; http://llvm.org/PR19051. Minor code-motion difference with -g.
+; Presence of debug info shouldn't affect the codegen. Make sure that
+; we generated the same code sequence with and without debug info. 
+;
+; CHECK:      callq   _Z3fooPcjPKc
+; CHECK:      callq   _Z3fooPcjPKc
+; CHECK:      leaq    (%rsp), %rdi
+; CHECK:      movl    $4, %esi
+; CHECK:      testl   {{%[a-z]+}}, {{%[a-z]+}}
+; CHECK:      je     .LBB0_4
+
+; Regenerate test with this command: 
+;   clang -emit-llvm -S -O2 -g
+; from this source:
+;
+; extern void foo(char *dst,unsigned siz,const char *src);
+; extern const char * i2str(int);
+;
+; struct AAA3 {
+;  AAA3(const char *value) { foo(text,sizeof(text),value);}
+;  void operator=(const char *value) { foo(text,sizeof(text),value);}
+;  operator const char*() const { return text;}
+;  char text[4];
+; };
+;
+; void bar (int param1,int param2)  {
+;   const char * temp(0);
+;
+;   if (param2) {
+;     temp = i2str(param2);
+;   }
+;   AAA3 var1("");
+;   AAA3 var2("");
+;
+;   if (param1)
+;     var2 = "+";
+;   else
+;     var2 = "-";
+;   var1 = "";
+; }
+
+%struct.AAA3 = type { [4 x i8] }
+
+@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
+@.str1 = private unnamed_addr constant [2 x i8] c"+\00", align 1
+@.str2 = private unnamed_addr constant [2 x i8] c"-\00", align 1
+
+; Function Attrs: uwtable
+define void @_Z3barii(i32 %param1, i32 %param2) #0 {
+entry:
+  %var1 = alloca %struct.AAA3, align 1
+  %var2 = alloca %struct.AAA3, align 1
+  %tobool = icmp eq i32 %param2, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  %call = call i8* @_Z5i2stri(i32 %param2)
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  call void @llvm.dbg.value(metadata !{%struct.AAA3* %var1}, i64 0, metadata !60)
+  call void @llvm.dbg.value(metadata !62, i64 0, metadata !63)
+  %arraydecay.i = getelementptr inbounds %struct.AAA3* %var1, i64 0, i32 0, i64 0
+  call void @_Z3fooPcjPKc(i8* %arraydecay.i, i32 4, i8* getelementptr inbounds ([1 x i8]* @.str, i64 0, i64 0))
+  call void @llvm.dbg.declare(metadata !{%struct.AAA3* %var2}, metadata !38)
+  %arraydecay.i5 = getelementptr inbounds %struct.AAA3* %var2, i64 0, i32 0, i64 0
+  call void @_Z3fooPcjPKc(i8* %arraydecay.i5, i32 4, i8* getelementptr inbounds ([1 x i8]* @.str, i64 0, i64 0))
+  %tobool1 = icmp eq i32 %param1, 0
+  br i1 %tobool1, label %if.else, label %if.then2
+
+if.then2:                                         ; preds = %if.end
+  call void @_Z3fooPcjPKc(i8* %arraydecay.i5, i32 4, i8* getelementptr inbounds ([2 x i8]* @.str1, i64 0, i64 0))
+  br label %if.end3
+
+if.else:                                          ; preds = %if.end
+  call void @_Z3fooPcjPKc(i8* %arraydecay.i5, i32 4, i8* getelementptr inbounds ([2 x i8]* @.str2, i64 0, i64 0))
+  br label %if.end3
+
+if.end3:                                          ; preds = %if.else, %if.then2
+  call void @_Z3fooPcjPKc(i8* %arraydecay.i, i32 4, i8* getelementptr inbounds ([1 x i8]* @.str, i64 0, i64 0))
+  ret void
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata) #1
+
+declare i8* @_Z5i2stri(i32) #2
+
+declare void @_Z3fooPcjPKc(i8*, i32, i8*) #2
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata) #1
+
+attributes #0 = { uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+attributes #2 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.module.flags = !{!48, !49}
+!llvm.ident = !{!50}
+
+!38 = metadata !{i32 786688, null, metadata !"var2", null, i32 20, null, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [var2] [line 20]
+!48 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
+!49 = metadata !{i32 1, metadata !"Debug Info Version", i32 1}
+!50 = metadata !{metadata !"clang version 3.5 (202418)"}
+!60 = metadata !{i32 786689, null, metadata !"this", null, i32 16777216, null, i32 1088, null} ; [ DW_TAG_arg_variable ] [this] [line 0]
+!62 = metadata !{i8* getelementptr inbounds ([1 x i8]* @.str, i64 0, i64 0)}
+!63 = metadata !{i32 786689, null, metadata !"value", null, i32 33554439, null, i32 0, null} ; [ DW_TAG_arg_variable ] [value] [line 7]