This is an archive of the discontinued LLVM Phabricator instance.

[NFC][NewGVN][LoadCoercion] Add new tests for future commit
AcceptedPublic

Authored by kmitropoulou on Apr 21 2022, 10:33 PM.

Diff Detail

Event Timeline

kmitropoulou created this revision.Apr 21 2022, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 10:33 PM
kmitropoulou requested review of this revision.Apr 21 2022, 10:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 10:33 PM
foad added a subscriber: foad.Apr 21 2022, 11:40 PM

Updating D124228: [NewGVN][LoadCoercion] Add new tests for future commit.

[NewGVN][LoadCoercion] Add new tests for future commit.

kmitropoulou retitled this revision from [NewGVN][LoadCoercion] Add new tests for future commit. to [NFC][NewGVN][LoadCoercion] Add new tests for future commit.Jun 13 2022, 2:57 AM

There's no tradition in sharing tests between GVN & NewGVN. Are you going to implement the functionality in both new & old GVN?

Plus, we would appreciate if you used poison instead of undef. Thank you!

foad accepted this revision.Jun 13 2022, 2:59 AM

LGTM. Could also use a common "GVN" prefix if you want (and if there are any tests at all where old and new gvn produce the same result).

This revision is now accepted and ready to land.Jun 13 2022, 2:59 AM

Updating D124228: [NFC][NewGVN][LoadCoercion] Add new tests for future commit

There's no tradition in sharing tests between GVN & NewGVN. Are you going to implement the functionality in both new & old GVN?

Plus, we would appreciate if you used poison instead of undef. Thank you!

I replaced all the undef values with poison.

To my understanding the new and old gvn have different lit tests. The old GVN already supports load coercion.

LGTM. Could also use a common "GVN" prefix if you want (and if there are any tests at all where old and new gvn produce the same result).

I am not sure what you mean. The old and new GVN might not have exactly the same output for load coercion e.g. in https://reviews.llvm.org/D124230 - test5, test 6 and test8 .

foad added a comment.Jun 15 2022, 4:25 AM

LGTM. Could also use a common "GVN" prefix if you want (and if there are any tests at all where old and new gvn produce the same result).

I am not sure what you mean. The old and new GVN might not have exactly the same output for load coercion e.g. in https://reviews.llvm.org/D124230 - test5, test 6 and test8 .

I mean if you write the RUN lines like this then update_test_checks is clever enough to use the GVN prefix if both passes produce identical results for a particular function, or separate OLDGVN and NEWGVN and prefixes if they produce different results:

; RUN: opt -S -gvn < %s | FileCheck %s -check-prefixes=GVN,OLDGVN
; RUN: opt -S -newgvn < %s | FileCheck %s -check-prefixes=GVN,NEWGVN

For example, at the moment I get different results on test1:

define i8 @test1(i32* %P1) {
; OLDGVN-LABEL: @test1(
; OLDGVN-NEXT:    [[V1:%.*]] = load i32, i32* [[P1:%.*]], align 4
; OLDGVN-NEXT:    [[P2:%.*]] = bitcast i32* [[P1]] to i8*
; OLDGVN-NEXT:    [[TMP1:%.*]] = trunc i32 [[V1]] to i8
; OLDGVN-NEXT:    [[V4:%.*]] = add i8 [[TMP1]], [[TMP1]]
; OLDGVN-NEXT:    ret i8 [[V4]]
;
; NEWGVN-LABEL: @test1(
; NEWGVN-NEXT:    [[V1:%.*]] = load i32, i32* [[P1:%.*]], align 4
; NEWGVN-NEXT:    [[P2:%.*]] = bitcast i32* [[P1]] to i8*
; NEWGVN-NEXT:    [[V2:%.*]] = load i8, i8* [[P2]], align 1
; NEWGVN-NEXT:    [[V3:%.*]] = trunc i32 [[V1]] to i8
; NEWGVN-NEXT:    [[V4:%.*]] = add i8 [[V2]], [[V3]]
; NEWGVN-NEXT:    ret i8 [[V4]]
;
  %V1 = load i32, i32* %P1
  %P2 = bitcast i32* %P1 to i8*
  %V2 = load i8, i8* %P2
  %V3 = trunc i32 %V1 to i8
  %V4 = add i8 %V2, %V3
  ret i8 %V4
}

but identical results on test4:

define <{<2 x i32>, i32}> @test4(i8* %P) {
; GVN-LABEL: @test4(
; GVN-NEXT:  Entry:
; GVN-NEXT:    [[P2:%.*]] = bitcast i8* [[P:%.*]] to i32*
; GVN-NEXT:    [[V2:%.*]] = load i32, i32* [[P2]], align 4
; GVN-NEXT:    [[P1:%.*]] = bitcast i8* [[P]] to <2 x i32>*
; GVN-NEXT:    [[V1:%.*]] = load <2 x i32>, <2 x i32>* [[P1]], align 8
; GVN-NEXT:    [[I1:%.*]] = insertvalue <{ <2 x i32>, i32 }> poison, <2 x i32> [[V1]], 0
; GVN-NEXT:    [[I2:%.*]] = insertvalue <{ <2 x i32>, i32 }> [[I1]], i32 [[V2]], 1
; GVN-NEXT:    ret <{ <2 x i32>, i32 }> [[I2]]
;
Entry:
  %P2 = bitcast i8* %P to i32*
  %V2 = load i32, i32* %P2
  %P1 = bitcast i8* %P to <2 x i32>*
  %V1 = load <2 x i32>, <2 x i32>* %P1
  %I1 = insertvalue <{<2 x i32>, i32}> poison, <2 x i32> %V1, 0
  %I2 = insertvalue <{<2 x i32>, i32}> %I1, i32 %V2, 1
  ret <{<2 x i32>, i32}> %I2
}

I think this is nice because it is really easy to see where the remaining differences are.

Updating D124228: [NFC][NewGVN][LoadCoercion] Add new tests for future commit

LGTM. Could also use a common "GVN" prefix if you want (and if there are any tests at all where old and new gvn produce the same result).

I am not sure what you mean. The old and new GVN might not have exactly the same output for load coercion e.g. in https://reviews.llvm.org/D124230 - test5, test 6 and test8 .

I mean if you write the RUN lines like this then update_test_checks is clever enough to use the GVN prefix if both passes produce identical results for a particular function, or separate OLDGVN and NEWGVN and prefixes if they produce different results:

; RUN: opt -S -gvn < %s | FileCheck %s -check-prefixes=GVN,OLDGVN
; RUN: opt -S -newgvn < %s | FileCheck %s -check-prefixes=GVN,NEWGVN

For example, at the moment I get different results on test1:

define i8 @test1(i32* %P1) {
; OLDGVN-LABEL: @test1(
; OLDGVN-NEXT:    [[V1:%.*]] = load i32, i32* [[P1:%.*]], align 4
; OLDGVN-NEXT:    [[P2:%.*]] = bitcast i32* [[P1]] to i8*
; OLDGVN-NEXT:    [[TMP1:%.*]] = trunc i32 [[V1]] to i8
; OLDGVN-NEXT:    [[V4:%.*]] = add i8 [[TMP1]], [[TMP1]]
; OLDGVN-NEXT:    ret i8 [[V4]]
;
; NEWGVN-LABEL: @test1(
; NEWGVN-NEXT:    [[V1:%.*]] = load i32, i32* [[P1:%.*]], align 4
; NEWGVN-NEXT:    [[P2:%.*]] = bitcast i32* [[P1]] to i8*
; NEWGVN-NEXT:    [[V2:%.*]] = load i8, i8* [[P2]], align 1
; NEWGVN-NEXT:    [[V3:%.*]] = trunc i32 [[V1]] to i8
; NEWGVN-NEXT:    [[V4:%.*]] = add i8 [[V2]], [[V3]]
; NEWGVN-NEXT:    ret i8 [[V4]]
;
  %V1 = load i32, i32* %P1
  %P2 = bitcast i32* %P1 to i8*
  %V2 = load i8, i8* %P2
  %V3 = trunc i32 %V1 to i8
  %V4 = add i8 %V2, %V3
  ret i8 %V4
}

but identical results on test4:

define <{<2 x i32>, i32}> @test4(i8* %P) {
; GVN-LABEL: @test4(
; GVN-NEXT:  Entry:
; GVN-NEXT:    [[P2:%.*]] = bitcast i8* [[P:%.*]] to i32*
; GVN-NEXT:    [[V2:%.*]] = load i32, i32* [[P2]], align 4
; GVN-NEXT:    [[P1:%.*]] = bitcast i8* [[P]] to <2 x i32>*
; GVN-NEXT:    [[V1:%.*]] = load <2 x i32>, <2 x i32>* [[P1]], align 8
; GVN-NEXT:    [[I1:%.*]] = insertvalue <{ <2 x i32>, i32 }> poison, <2 x i32> [[V1]], 0
; GVN-NEXT:    [[I2:%.*]] = insertvalue <{ <2 x i32>, i32 }> [[I1]], i32 [[V2]], 1
; GVN-NEXT:    ret <{ <2 x i32>, i32 }> [[I2]]
;
Entry:
  %P2 = bitcast i8* %P to i32*
  %V2 = load i32, i32* %P2
  %P1 = bitcast i8* %P to <2 x i32>*
  %V1 = load <2 x i32>, <2 x i32>* %P1
  %I1 = insertvalue <{<2 x i32>, i32}> poison, <2 x i32> %V1, 0
  %I2 = insertvalue <{<2 x i32>, i32}> %I1, i32 %V2, 1
  ret <{<2 x i32>, i32}> %I2
}

I think this is nice because it is really easy to see where the remaining differences are.

Thanks Jay :) I am going to update all the tests.

Updating D124228: [NFC][NewGVN][LoadCoercion] Add new tests for future commit

Updating D124228: [NFC][NewGVN][LoadCoercion] Add new tests for future commit