Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GVN incorrectly changes a load into a bitcast #48826

Closed
wsmoses opened this issue Mar 9, 2021 · 8 comments
Closed

GVN incorrectly changes a load into a bitcast #48826

wsmoses opened this issue Mar 9, 2021 · 8 comments
Assignees
@jdoerfert
Labels
bugzilla Issues migrated from bugzilla

Comments

@wsmoses
Copy link
Member

wsmoses commented Mar 9, 2021

Bugzilla Link 49482
Resolution FIXED
Resolved on Jun 03, 2021 23:16
Version trunk
OS All
Blocks #48661 #48289
CC @jdoerfert,@tstellar,@wsmoses
Fixed by commit(s) 875891a 77b63ce

Extended Description

GVN has emits incorrect results when given an invariant.group

; ModuleID = 'tc.ll'
source_filename = "Main.cpp"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%struct.c = type { double, double }

@.str = private unnamed_addr constant [4 x i8] c"%f\0A\00", align 1

; Function Attrs: norecurse uwtable mustprogress
define dso_local i32 @​main() local_unnamed_addr #​0 {
entry:
  %call = tail call noalias dereferenceable_or_null(12297664) i8* @​malloc(i64 12297664) #​3
  %0 = bitcast i8* %call to %struct.c*
  %call1 = tail call noalias dereferenceable_or_null(12297664) i8* @​malloc(i64 12297664) #​3
  br label %for.body

for.body:                                         ; preds = %for.body, %entry
  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
  %tr = trunc i64 %indvars.iv to i32
  %conv = sitofp i32 %tr to double
  %a = getelementptr inbounds %struct.c, %struct.c* %0, i64 %indvars.iv, i32 0
  store double %conv, double* %a, align 8
  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
  %exitcond.not = icmp eq i64 %indvars.iv.next, 768604
  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body, !llvm.loop !​2

for.cond.cleanup:                                 ; preds = %for.body
  %arrayidx7.i.i = getelementptr inbounds i8, i8* %call, i64 10489184
  %malloccall19.i = call noalias nonnull dereferenceable(16) dereferenceable_or_null(16) i8* @​malloc(i64 16) #​3, !noalias !​4
  %_malloccache.i = bitcast i8* %malloccall19.i to %struct.c**
  %malloccall23.i = call noalias nonnull dereferenceable(16) dereferenceable_or_null(16) i8* @​malloc(i64 16) #​3, !noalias !​4
  br label %for.body.i

for.body.i:                                       ; preds = %for.body.i, %for.cond.cleanup
  %iv.i = phi i64 [ 0, %for.cond.cleanup ], [ %iv.next.i, %for.body.i ]
  %1 = phi double [ 0.000000e+00, %for.cond.cleanup ], [ %add.i, %for.body.i ]
  %iv.next.i = add nuw nsw i64 %iv.i, 1
  %2 = bitcast i8* %arrayidx7.i.i to %struct.c*
  %3 = getelementptr inbounds %struct.c*, %struct.c** %_malloccache.i, i64 %iv.i
  store %struct.c* %2, %struct.c** %3, align 8, !invariant.group !​7
  %a10.i.i = getelementptr inbounds %struct.c, %struct.c* %2, i64 0, i32 0
  %4 = load double, double* %a10.i.i, align 8, !invariant.group !​8
  %sub.i.i = fsub double 1.000000e+00, %4
  %div11.i.i = fdiv double 1.000000e+00, %sub.i.i
  %add.i = fadd double %1, %div11.i.i
  %exitcond.i.not = icmp eq i64 %iv.next.i, 2
  br i1 %exitcond.i.not, label %diffe_Z1pP1cPd.exit, label %for.body.i, !llvm.loop !​9

diffe_Z1pP1cPd.exit:                              ; preds = %for.body.i
  %call4 = call i32 (i8*, ...) @​printf(i8* nonnull dereferenceable(1) getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), double %add.i)
  ret i32 0
}

; Function Attrs: inaccessiblememonly nofree nounwind willreturn
declare dso_local noalias noundef i8* @​malloc(i64 noundef) local_unnamed_addr #​1

; Function Attrs: nofree nounwind
declare dso_local noundef i32 @​printf(i8* nocapture noundef readonly, ...) local_unnamed_addr #​2

attributes #​0 = { norecurse uwtable mustprogress "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #​1 = { inaccessiblememonly nofree nounwind willreturn "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #​2 = { nofree nounwind "frame-pointer"="none" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #​3 = { nounwind }

!llvm.module.flags = !{#0}
!llvm.ident = !{#1}

!​0 = !{i32 1, !"wchar_size", i32 4}
!​1 = !{!"clang version 13.0.0 (git%github.com:llvm/llvm-project d163e75c81c1609855b98cbdffa0141c70d8578d)"}
!​2 = distinct !{#2, !​3}
!​3 = !{!"llvm.loop.mustprogress"}
!​4 = !{#5}
!​5 = distinct !{#5, !​6, !"diffe_Z1pP1cPd: %q"}
!​6 = distinct !{#6, !"diffe_Z1pP1cPd"}
!​7 = distinct !{}
!​8 = distinct !{}
!​9 = distinct !{#9, !​3}

$ /mnt/sabrent/wmoses/llvm13/build/bin/opt tc.ll | /mnt/sabrent/wmoses/llvm13/build/bin/lli -
-0.000003

$ /mnt/sabrent/wmoses/llvm13/build/bin/opt -gvn tc.ll | /mnt/sabrent/wmoses/llvm13/build/bin/lli -
2.000000

Removing one of the two invariant.groups appears to fix the incorrect behavior of gvn.
@wsmoses
Copy link
Member Author

wsmoses commented Mar 9, 2021

assigned to @jdoerfert

@wsmoses
Copy link
Member Author

wsmoses commented Mar 9, 2021

Further reducing this test case, GVN incorrectly changes the named "iload" below into a bitcast rather than actually loading.

; ModuleID = 'tc.ll'
source_filename = "Main.cpp"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%struct.c = type { double, double }

@.str = private unnamed_addr constant [4 x i8] c"%f\0A\00", align 1

; Function Attrs: norecurse uwtable mustprogress
define i32 @​main() local_unnamed_addr {
entry:
  %call = tail call noalias i8* @​malloc(i64 32)
  %0 = bitcast i8* %call to %struct.c*
  %a1 = getelementptr inbounds %struct.c, %struct.c* %0, i64 1, i32 0
  store double 1.23450000e+00, double* %a1, align 8
  %arrayidx7.i.i = getelementptr inbounds i8, i8* %call, i64 16
  %malloccall19.i = call noalias i8* @​malloc(i64 8)
  %_malloccache.i = bitcast i8* %malloccall19.i to %struct.c**
  %c2 = bitcast i8* %arrayidx7.i.i to %struct.c*
  %g3 = getelementptr inbounds %struct.c*, %struct.c** %_malloccache.i, i64 0
  store %struct.c* %c2, %struct.c** %g3, align 8, !invariant.group !​7
  %a10.i.i = getelementptr inbounds %struct.c, %struct.c* %c2, i64 0, i32 0
  %iload = load double, double* %a10.i.i, align 8, !invariant.group !​8
  %call4 = call i32 (i8*, ...) @​printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), double %iload)
  ret i32 0
}

; Function Attrs: inaccessiblememonly nofree nounwind willreturn
declare dso_local noalias noundef i8* @​malloc(i64 noundef)

; Function Attrs: nofree nounwind
declare dso_local noundef i32 @​printf(i8* nocapture noundef readonly, ...)

!llvm.module.flags = !{#0}
!llvm.ident = !{#1}

!​0 = !{i32 1, !"wchar_size", i32 4}
!​1 = !{!"clang version 13.0.0 (git%github.com:llvm/llvm-project d163e75c81c1609855b98cbdffa0141c70d8578d)"}
!​2 = distinct !{#2, !​3}
!​3 = !{!"llvm.loop.mustprogress"}
!​4 = !{#5}
!​5 = distinct !{#5, !​6, !"diffe_Z1pP1cPd: %q"}
!​6 = distinct !{#6, !"diffe_Z1pP1cPd"}
!​7 = distinct !{}
!​8 = distinct !{}
!​9 = distinct !{#9, !​3}

$ /mnt/sabrent/wmoses/llvm13/build/bin/opt tc.ll | /mnt/sabrent/wmoses/llvm13/build/bin/lli -
1.234500

$ /mnt/sabrent/wmoses/llvm13/build/bin/opt -gvn tc.ll | /mnt/sabrent/wmoses/llvm13/build/bin/lli -
0.000000

$ /mnt/sabrent/wmoses/llvm13/build/bin/opt -gvn tc.ll -S
; ModuleID = 'tc.ll'
source_filename = "Main.cpp"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%struct.c = type { double, double }

@.str = private unnamed_addr constant [4 x i8] c"%f\0A\00", align 1

define i32 @​main() local_unnamed_addr {
entry:
  %call = tail call noalias i8* @​malloc(i64 32)
  %0 = bitcast i8* %call to %struct.c*
  %a1 = getelementptr inbounds %struct.c, %struct.c* %0, i64 1, i32 0
  store double 1.234500e+00, double* %a1, align 8
  %arrayidx7.i.i = getelementptr inbounds i8, i8* %call, i64 16
  %malloccall19.i = call noalias i8* @​malloc(i64 8)
  %_malloccache.i = bitcast i8* %malloccall19.i to %struct.c**
  %c2 = bitcast i8* %arrayidx7.i.i to %struct.c*
  store %struct.c* %c2, %struct.c** %_malloccache.i, align 8, !invariant.group !​2
  %a10.i.i = getelementptr inbounds %struct.c, %struct.c* %c2, i64 0, i32 0
  %1 = ptrtoint %struct.c* %c2 to i64
  %2 = bitcast i64 %1 to double
  %call4 = call i32 (i8*, ...) @​printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), double %2)
  ret i32 0
}

@wsmoses
Copy link
Member Author

wsmoses commented Mar 10, 2021

Fixed by 875891a

@slacka
Copy link
Mannequin

slacka mannequin commented Apr 27, 2021

Reopened for 12.0.1 backport.

@tstellar
Copy link
Collaborator

Hi Johannes,

What is your opinion on backporting this?

https://reviews.llvm.org/rG875891a10d50a791d3f076c9259e60af6c9af18c

@wsmoses
Copy link
Member Author

wsmoses commented May 7, 2021

I'm obviously not Johannes, but I would definitely be in favor of backporting this.

@jdoerfert
Copy link
Member

Seems pretty safe to me and fixes a problem, so yes.

@tstellar
Copy link
Collaborator

tstellar commented Jun 4, 2021

Merged: 77b63ce

This was referenced Jan 29, 2021
Closed
Closed
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jdoerfert jdoerfert

Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tstellar @wsmoses @jdoerfert