Updating D145990: [AMDGPU] Update mul.ll with auto-generated checks

Updating D145990: [AMDGPU] Update mul.ll with auto-generated checks

Updating D145990: [AMDGPU] Update mul.ll with auto-generated checks

Updating D144313: [AMDGPU] Improve the lowering of raw_buffer_load_{i8,i16} and struct_buffer_load_{i8,i16} intrinsics

Updating D144313: [AMDGPU] Improve the lowering of raw_buffer_load_{i8,i16} and struct_buffer_load_{i8,i16} intrinsics

Updating D144312: [AMDGPU] Add tests for future commit

Updating D144313: [AMDGPU] Improve the lowering of raw_buffer_load_{i8,i16} and struct_buffer_load_{i8,i16} intrinsics

Updating D144313: [AMDGPU] Improve the lowering of raw_buffer_load_{i8,i16} and struct_buffer_load_{i8,i16} intrinsics

Updating D144313: [AMDGPU] Improve the lowering of buffer_load_{u8, u16} intrinsics.

Updating D144313: [AMDGPU] Improve the lowering of buffer_load_{u8, u16} intrinsics.

We can revisit this patch if the problem shows up again.

I cannot reproduce the bug. I had to decrease the counter down to 6 in order to activate the assertion. This is normal. The counter is updated after value numbering. An instruction will be processed again if one of its operands is added in a different congruence class. Do you remember which was the problem?

In D136164#3882569, @nikic wrote:

Ping :)

In D136164#3865041, @foad wrote:

e.g. for NewGVN I think we should be able to use a single BatchAA instance for all queries.

@kmitropoulou it sounds like this could help with NewGVN compile time.

Without looking too deeply into it, it's likely that the changes proposed by @kmitropoulou would actually prevent sharing BatchAA for all queries, because they introduce IR changes to the analysis phase, while NewGVN is generally designed to separate analysis and transformation.

Updating D127628: [NewGVN][LoadCoercion][3/3] Partial redundant load elimination for load instructions that are dependent on a MemoryPhi or another load instruction in a predecessor basic block.

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

In D130949#3803992, @asbirlea wrote:

Can you rebase the patch on ToT then?

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

Updating D127628: [NewGVN][LoadCoercion][3/3] Partial redundant load elimination for load instructions that are dependent on a MemoryPhi or another load instruction in a predecessor basic block.

Updating D124230: [NewGVN][LoadCoercion][2/3] Add support for load coercion between load instructions.

Updating D124071: [NewGVN][LoadCoercion][1/3] Add support for load coercion between store and load instructions

Updating D127628: [NewGVN][LoadCoercion][3/3] Partial redundant load elimination for load instructions that are dependent on a MemoryPhi or another load instruction in a predecessor basic block.

Updating D127628: [NewGVN][LoadCoercion][3/3] Partial redundant load elimination for load instructions that are dependent on a MemoryPhi or another load instruction in a predecessor basic block.

Updating D124230: [NewGVN][LoadCoercion][2/3] Add support for load coercion between load instructions.

Updating D124071: [NewGVN][LoadCoercion][1/3] Add support for load coercion between store and load instructions

Updating D127628: [NewGVN][LoadCoercion][3/3] Partial redundant load elimination for load instructions that are dependent on a MemoryPhi or another load instruction in a predecessor basic block.

Updating D124230: [NewGVN][LoadCoercion][2/3] Add support for load coercion between load instructions.

Updating D124071: [NewGVN][LoadCoercion][1/3] Add support for load coercion between store and load instructions

Updating D124069: [NFC][NewGVN] Add updateDFSNumbers().

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

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

Updating D124069: [NFC][NewGVN] Add updateDFSNumbers().

Updating D124068: [NFC][NewGVN][LoadCoercion] Add tests for future commit.

In D127628#3738907, @foad wrote:

In example 2 you insert a new instruction %V1 = load i32, i32* %P at the end of BB2. I'm not sure this is safe in general. For example, if BB2 has other successors:

BB1     BB2
   \   /   \
    BB3     BB4

then the new load instruction will be executed even if BB2 is going to branch to BB4, and in that case the load might trap.

Updating D127628: [NewGVN][LoadCoercion][3/3] Replace load with a phi node

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

In D130949#3732343, @fhahn wrote:

Could you elaborate on the motivation for removing the helper?

Updating D130910: [NewGVN][PHIOFOPS] Bail out if an operand is in OpSafeForPHIOfOps but it is not safe for the current basic block.

ping

Updating D130910: [NewGVN][PHIOFOPS] Bail out if an operand is in OpSafeForPHIOfOps but it is not safe for the current basic block.

ping

ping

In D130910#3692890, @foad wrote:

I am not too familiar with NewGVN, but wouldn't it be simpler and safer to clear OpSafeForPHIOfOps in verifyIterationSettled()? It is inside #ifndef NDEBUG so it doesn't matter too much if it's a bit slower.

Updating D130910: [NewGVN][PHIOFOPS] Bail-out if an operand is in OpSafeForPHIOfOps but it is not safe for the current basic block.

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

ping

ping

ping

Updating D127628: [NewGVN][LoadCoercion][3/3] Replace load with a phi node

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

Updating D127628: [NewGVN][LoadCoercion][3/3] Replace load with a phi node

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

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

Updating D124068: [NFC][NewGVN][LoadCoercion] Add tests for future commit.

Updating D127628: [NewGVN][LoadCoercion][3/3] Replace load with a phi node

Updating D124230: [NewGVN][LoadCoercion][2/3] Add support for load coercion between load instructions.

Updating D124071: [NewGVN][LoadCoercion][1/3] Add support for load coercion between store and load instructions

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

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

In D124228#3584866, @foad wrote:

In D124228#3583946, @kmitropoulou wrote:

In D124228#3577317, @foad wrote:

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

In D124228#3577317, @foad wrote:

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).

In D124228#3577315, @nlopes wrote:

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!