Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-15 Thread Wei Mi via cfe-commits 
On Mon, Jun 14, 2021 at 4:52 PM Wei Mi  wrote:

>
>
> On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li 
> wrote:
>
>>
>>
>> On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> lebedev.ri added a subscriber: MaskRay.
>>> lebedev.ri added a comment.
>>>
>>> In D104099#2815531 , @wenlei
>>> wrote:
>>>
>>> > In D104099#2814167 ,
>>> @davidxl wrote:
>>> >
>>> >> Adding Wei to help measure performance impact on our internal
>>> workloads.  Also add Wenlei to help measure impact with FB's workloads.
>>> >
>>> > Measured perf using FB internal workload w/ and w/o this pass, result
>>> is neutral.
>>>
>>> Thank you for checking!
>>>
>>> So far, it seems the reaction to this proposal has been overwhelmingly
>>> positive.
>>> Does anyone else wish to chime in? Should i land this? @asbirlea
>>> @MaskRay ?
>>>
>>
>> Wei is doing more measurement @google. Please wait for the response.
>>
>> David
>>
>
> Start doing the test. Will report back.
>
> Wei.
>

No performance change found in google internal benchmarks.

 Wei.


>
>
>>
>>
>>>
>>> Repository:
>>>   rG LLVM Github Monorepo
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D104099/new/
>>>
>>> https://reviews.llvm.org/D104099
>>>
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-14 Thread Wei Mi via cfe-commits 
On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li 
wrote:

>
>
> On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> lebedev.ri added a subscriber: MaskRay.
>> lebedev.ri added a comment.
>>
>> In D104099#2815531 , @wenlei
>> wrote:
>>
>> > In D104099#2814167 ,
>> @davidxl wrote:
>> >
>> >> Adding Wei to help measure performance impact on our internal
>> workloads.  Also add Wenlei to help measure impact with FB's workloads.
>> >
>> > Measured perf using FB internal workload w/ and w/o this pass, result
>> is neutral.
>>
>> Thank you for checking!
>>
>> So far, it seems the reaction to this proposal has been overwhelmingly
>> positive.
>> Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay
>> ?
>>
>
> Wei is doing more measurement @google. Please wait for the response.
>
> David
>

Start doing the test. Will report back.

Wei.


>
>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D104099/new/
>>
>> https://reviews.llvm.org/D104099
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-14 Thread Xinliang David Li via cfe-commits 
On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:

> lebedev.ri added a subscriber: MaskRay.
> lebedev.ri added a comment.
>
> In D104099#2815531 , @wenlei
> wrote:
>
> > In D104099#2814167 , @davidxl
> wrote:
> >
> >> Adding Wei to help measure performance impact on our internal
> workloads.  Also add Wenlei to help measure impact with FB's workloads.
> >
> > Measured perf using FB internal workload w/ and w/o this pass, result is
> neutral.
>
> Thank you for checking!
>
> So far, it seems the reaction to this proposal has been overwhelmingly
> positive.
> Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay ?
>

Wei is doing more measurement @google. Please wait for the response.

David


>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D104099/new/
>
> https://reviews.llvm.org/D104099
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-14 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a subscriber: MaskRay.
lebedev.ri added a comment.

In D104099#2815531 , @wenlei wrote:

> In D104099#2814167 , @davidxl wrote:
>
>> Adding Wei to help measure performance impact on our internal workloads.  
>> Also add Wenlei to help measure impact with FB's workloads.
>
> Measured perf using FB internal workload w/ and w/o this pass, result is 
> neutral.

Thank you for checking!

So far, it seems the reaction to this proposal has been overwhelmingly positive.
Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-12 Thread Wenlei He via Phabricator via cfe-commits 
wenlei added a comment.

In D104099#2814167 , @davidxl wrote:

> Adding Wei to help measure performance impact on our internal workloads.  
> Also add Wenlei to help measure impact with FB's workloads.

Measured perf using FB internal workload w/ and w/o this pass, result is 
neutral.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread David Li via Phabricator via cfe-commits 
davidxl added a comment.

Adding Wei to help measure performance impact on our internal workloads.  Also 
add Wenlei to help measure impact with FB's workloads.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks accepted this revision.
aeubanks added a comment.
This revision is now accepted and ready to land.

Ah, sorry I missed where you mentioned LSR.

If this pass is causing regressions in multiple places, even on X86, then I 
think it does make sense to remove it.
I've added some people that may care about this pass more than me. Maybe wait a 
bit for their feedback, if they have any.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

In D104099#2813743 , @aeubanks wrote:

> Which pass that comes after SpeculateAroundPHIs in the X86 pipeline (either 
> in the optimization or codegen) would undo its effects?

As i have wrote in some other review, the pass i saw as causing problems is LSR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

Which pass that comes after SpeculateAroundPHIs in the X86 pipeline (either in 
the optimization or codegen) would undo its effects?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

In D104099#2813713 , @aeubanks wrote:

> Some backends don't run SimplifyCFG, e.g. X86. I believe the pass was 
> originally created specifically for X86 (the header has some X86 examples) 
> and may or may not extend to other targets (I'm not very familiar with the 
> pass itself).
>
> I'm not opposed to landing this and seeing who complains, but if somebody 
> does, we can make this pass X86-specific by adding it to 
> X86TargetMachine::registerPassBuilderCallbacks() (which doesn't exist yet).

I am complaining about X86 side of things :)
It needs to be *at least* a late IR pass in codegen pipeline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Arthur Eubanks via Phabricator via cfe-commits 
aeubanks added a comment.

Some backends don't run SimplifyCFG, e.g. X86. I believe the pass was 
originally created specifically for X86 (the header has some X86 examples) and 
may or may not extend to other targets (I'm not very familiar with the pass 
itself).

I'm not opposed to landing this and seeing who complains, but if somebody does, 
we can make this pass X86-specific by adding it to 
X86TargetMachine::registerPassBuilderCallbacks() (which doesn't exist yet).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri updated this revision to Diff 351445.
lebedev.ri added a comment.

... and upload the right patch this time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Transforms/LoopUnroll/AArch64/runtime-unroll-generic.ll
  
llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
  llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll

Index: llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
===
--- llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
+++ llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
@@ -104,21 +104,18 @@
 ; ROTATED_LATER_NEWPM-NEXT:br i1 [[CMP13_NOT]], label [[FOR_COND_CLEANUP:%.*]], label [[FOR_BODY_PREHEADER:%.*]]
 ; ROTATED_LATER_NEWPM:   for.body.preheader:
 ; ROTATED_LATER_NEWPM-NEXT:[[TMP0:%.*]] = add nsw i32 [[WIDTH]], -1
-; ROTATED_LATER_NEWPM-NEXT:[[INC_1:%.*]] = add nuw nsw i32 0, 1
 ; ROTATED_LATER_NEWPM-NEXT:br label [[FOR_BODY:%.*]]
 ; ROTATED_LATER_NEWPM:   for.cond.cleanup:
 ; ROTATED_LATER_NEWPM-NEXT:tail call void @f0()
 ; ROTATED_LATER_NEWPM-NEXT:tail call void @f2()
 ; ROTATED_LATER_NEWPM-NEXT:br label [[RETURN]]
 ; ROTATED_LATER_NEWPM:   for.body:
-; ROTATED_LATER_NEWPM-NEXT:[[INC_PHI:%.*]] = phi i32 [ [[INC_0:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ], [ [[INC_1]], [[FOR_BODY_PREHEADER]] ]
+; ROTATED_LATER_NEWPM-NEXT:[[I_04:%.*]] = phi i32 [ [[INC:%.*]], [[FOR_BODY]] ], [ 0, [[FOR_BODY_PREHEADER]] ]
 ; ROTATED_LATER_NEWPM-NEXT:tail call void @f0()
 ; ROTATED_LATER_NEWPM-NEXT:tail call void @f1()
-; ROTATED_LATER_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC_PHI]], [[TMP0]]
-; ROTATED_LATER_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]]
-; ROTATED_LATER_NEWPM:   for.body.for.body_crit_edge:
-; ROTATED_LATER_NEWPM-NEXT:[[INC_0]] = add nuw nsw i32 [[INC_PHI]], 1
-; ROTATED_LATER_NEWPM-NEXT:br label [[FOR_BODY]]
+; ROTATED_LATER_NEWPM-NEXT:[[INC]] = add nuw nsw i32 [[I_04]], 1
+; ROTATED_LATER_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC]], [[TMP0]]
+; ROTATED_LATER_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY]]
 ; ROTATED_LATER_NEWPM:   return:
 ; ROTATED_LATER_NEWPM-NEXT:ret void
 ;
@@ -155,21 +152,18 @@
 ; ROTATE_NEWPM-NEXT:br i1 [[CMP13_NOT]], label [[FOR_COND_CLEANUP:%.*]], label [[FOR_BODY_PREHEADER:%.*]]
 ; ROTATE_NEWPM:   for.body.preheader:
 ; ROTATE_NEWPM-NEXT:[[TMP0:%.*]] = add nsw i32 [[WIDTH]], -1
-; ROTATE_NEWPM-NEXT:[[INC_1:%.*]] = add nuw nsw i32 0, 1
 ; ROTATE_NEWPM-NEXT:br label [[FOR_BODY:%.*]]
 ; ROTATE_NEWPM:   for.cond.cleanup:
 ; ROTATE_NEWPM-NEXT:tail call void @f0()
 ; ROTATE_NEWPM-NEXT:tail call void @f2()
 ; ROTATE_NEWPM-NEXT:br label [[RETURN]]
 ; ROTATE_NEWPM:   for.body:
-; ROTATE_NEWPM-NEXT:[[INC_PHI:%.*]] = phi i32 [ [[INC_0:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ], [ [[INC_1]], [[FOR_BODY_PREHEADER]] ]
+; ROTATE_NEWPM-NEXT:[[I_04:%.*]] = phi i32 [ [[INC:%.*]], [[FOR_BODY]] ], [ 0, [[FOR_BODY_PREHEADER]] ]
 ; ROTATE_NEWPM-NEXT:tail call void @f0()
 ; ROTATE_NEWPM-NEXT:tail call void @f1()
-; ROTATE_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC_PHI]], [[TMP0]]
-; ROTATE_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]]
-; ROTATE_NEWPM:   for.body.for.body_crit_edge:
-; ROTATE_NEWPM-NEXT:[[INC_0]] = add nuw nsw i32 [[INC_PHI]], 1
-; ROTATE_NEWPM-NEXT:br label [[FOR_BODY]]
+; ROTATE_NEWPM-NEXT:[[INC]] = add nuw nsw i32 [[I_04]], 1
+; ROTATE_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC]], [[TMP0]]
+; ROTATE_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY]]
 ; ROTATE_NEWPM:   return:
 ; ROTATE_NEWPM-NEXT:ret void
 ;
Index: llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
===
--- llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
+++ llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
@@ -156,36 +156,27 @@
 ; CHECK:   vector.ph:
 ; CHECK-NEXT:[[BROADCAST_SPLATINSERT:%.*]] = insertelement <4 x float> poison, float [[X:%.*]], i32 0
 ; CHECK-NEXT:[[BROADCAST_SPLAT:%.*]] =

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri updated this revision to Diff 351444.
lebedev.ri added a comment.
Herald added a subscriber: zzheng.

Update a few more tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Transforms/LoopUnroll/AArch64/runtime-unroll-generic.ll
  
llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
  llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll

Index: llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
===
--- llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
+++ llvm/test/Transforms/PhaseOrdering/loop-rotation-vs-common-code-hoisting.ll
@@ -107 +106,0 @@
-; ROTATED_LATER_NEWPM-NEXT:[[INC_1:%.*]] = add nuw nsw i32 0, 1
@@ -114 +113 @@
-; ROTATED_LATER_NEWPM-NEXT:[[INC_PHI:%.*]] = phi i32 [ [[INC_0:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ], [ [[INC_1]], [[FOR_BODY_PREHEADER]] ]
+; ROTATED_LATER_NEWPM-NEXT:[[I_04:%.*]] = phi i32 [ [[INC:%.*]], [[FOR_BODY]] ], [ 0, [[FOR_BODY_PREHEADER]] ]
@@ -117,5 +116,3 @@
-; ROTATED_LATER_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC_PHI]], [[TMP0]]
-; ROTATED_LATER_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]]
-; ROTATED_LATER_NEWPM:   for.body.for.body_crit_edge:
-; ROTATED_LATER_NEWPM-NEXT:[[INC_0]] = add nuw nsw i32 [[INC_PHI]], 1
-; ROTATED_LATER_NEWPM-NEXT:br label [[FOR_BODY]]
+; ROTATED_LATER_NEWPM-NEXT:[[INC]] = add nuw nsw i32 [[I_04]], 1
+; ROTATED_LATER_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC]], [[TMP0]]
+; ROTATED_LATER_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY]]
@@ -158 +154,0 @@
-; ROTATE_NEWPM-NEXT:[[INC_1:%.*]] = add nuw nsw i32 0, 1
@@ -165 +161 @@
-; ROTATE_NEWPM-NEXT:[[INC_PHI:%.*]] = phi i32 [ [[INC_0:%.*]], [[FOR_BODY_FOR_BODY_CRIT_EDGE:%.*]] ], [ [[INC_1]], [[FOR_BODY_PREHEADER]] ]
+; ROTATE_NEWPM-NEXT:[[I_04:%.*]] = phi i32 [ [[INC:%.*]], [[FOR_BODY]] ], [ 0, [[FOR_BODY_PREHEADER]] ]
@@ -168,5 +164,3 @@
-; ROTATE_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC_PHI]], [[TMP0]]
-; ROTATE_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY_FOR_BODY_CRIT_EDGE]]
-; ROTATE_NEWPM:   for.body.for.body_crit_edge:
-; ROTATE_NEWPM-NEXT:[[INC_0]] = add nuw nsw i32 [[INC_PHI]], 1
-; ROTATE_NEWPM-NEXT:br label [[FOR_BODY]]
+; ROTATE_NEWPM-NEXT:[[INC]] = add nuw nsw i32 [[I_04]], 1
+; ROTATE_NEWPM-NEXT:[[EXITCOND_NOT:%.*]] = icmp eq i32 [[INC]], [[TMP0]]
+; ROTATE_NEWPM-NEXT:br i1 [[EXITCOND_NOT]], label [[FOR_COND_CLEANUP]], label [[FOR_BODY]]
Index: llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
===
--- llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
+++ llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-sinking-required-for-vectorization.ll
@@ -159,4 +158,0 @@
-; CHECK-NEXT:[[DOT0:%.*]] = getelementptr inbounds i32, i32* [[C]], i64 0
-; CHECK-NEXT:[[DOT017:%.*]] = getelementptr inbounds float, float* [[A]], i64 0
-; CHECK-NEXT:[[DOT018:%.*]] = getelementptr inbounds float, float* [[B]], i64 0
-; CHECK-NEXT:[[INDEX_NEXT_0:%.*]] = add nuw i64 0, 4
@@ -165,24 +161,19 @@
-; CHECK-NEXT:[[INDEX_NEXT_PHI:%.*]] = phi i64 [ [[INDEX_NEXT_0]], [[VECTOR_PH]] ], [ [[INDEX_NEXT_1:%.*]], [[VECTOR_BODY_VECTOR_BODY_CRIT_EDGE:%.*]] ]
-; CHECK-NEXT:[[DOTPHI:%.*]] = phi float* [ [[DOT018]], [[VECTOR_PH]] ], [ [[DOT120:%.*]], [[VECTOR_BODY_VECTOR_BODY_CRIT_EDGE]] ]
-; CHECK-NEXT:[[DOTPHI21:%.*]] = phi float* [ [[DOT017]], [[VECTOR_PH]] ], [ [[DOT119:%.*]], [[VECTOR_BODY_VECTOR_BODY_CRIT_EDGE]] ]
-; CHECK-NEXT:[[DOTPHI22:%.*]] = phi i32* [ [[DOT0]], [[VECTOR_PH]] ], [ [[DOT1:%.*]], [[VECTOR_BODY_VECTOR_BODY_CRIT_EDGE]] ]
-; CHECK-NEXT:[[TMP2:%.*]] = bitcast i32* [[DOTPHI22]] to <4 x i32>*
-; CHECK-NEXT:[[WIDE_LOAD:%.*]] = load <4 x i32>, <4 x i32>* [[TMP2]], align 4, !alias.scope !8
-; CHECK-NEXT:[[TMP3:%.*]] = icmp eq <4 x i32> [[WIDE_LOAD]], 
-; CHECK-NEXT:[[TMP4:%.*]] = bitcast float* [[DOTPHI21]] to <4 x float>*
-; CHECK-NEXT:[[WIDE_LOAD14:%.*]] = load <4 x float>, <4 x float>* [[TMP4]], align 4, !alias.scope !11
-; CHECK-NEXT:[[TMP5:%.*]] = fmul <4 x float> [[WIDE_LOAD14]], [[BROADCAST_SPLAT]]
-; CHECK-NEXT:[[TMP6:%.*]] = bitcast float* [[DOTPHI]] to <4 x float>*
-;

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Bjorn Pettersson via Phabricator via cfe-commits 
bjope added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1449
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-

lebedev.ri wrote:
> nikic wrote:
> > As it has been in-tree for a while, I think it would be good to add a 
> > cl::opt option for people who have come to rely on it being present in some 
> > way.
> I'm not convinced that the pass can be returned into it's current position in 
> pipeline,
> that is why the diff is what it is now.
> 
Yes, there is a contradiction between the code comment and the position in the 
pipeline. I did question that here: 
https://bugs.llvm.org/show_bug.cgi?id=48821#c2



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Bjorn Pettersson via Phabricator via cfe-commits 
bjope added a comment.

Kind of the same take on this as @thopre .

I wrote https://bugs.llvm.org/show_bug.cgi?id=48821 , and proposed 
https://reviews.llvm.org/D95789 , when I noticed that this pass caused troubles 
for two reasons:

1. to avoid that others would stumble upon the same problem (getting unexpected 
regressions due to switching pass manager)
2. not that important really, but getting rid of downstream diffs is always 
nice (at least when the problem is likely to cause problems for other targets 
as well)

For the downstream target I've hacked around this a long time ago.

I do think it was a bit sneaky to get this pass by default in the pipeline as 
part of the PM switch. It might have caused lots of trouble for the new-PM 
switch in the past, and still for some downstream contributors.
So in that sense I'm all in favor. Although, a LGTM from me is a bit weak as 
I'm already using a workaround to skip this pass downstream.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Thomas Preud'homme via Phabricator via cfe-commits 
thopre added a comment.

+1 for this change but being a downstream target only I prefer to let someone 
else approve this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1449
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-

nikic wrote:
> As it has been in-tree for a while, I think it would be good to add a cl::opt 
> option for people who have come to rely on it being present in some way.
I'm not convinced that the pass can be returned into it's current position in 
pipeline,
that is why the diff is what it is now.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Nikita Popov via Phabricator via cfe-commits 
nikic added a comment.

Looks like phase ordering tests need an update.




Comment at: llvm/lib/Passes/PassBuilder.cpp:1449
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-

As it has been in-tree for a while, I think it would be good to add a cl::opt 
option for people who have come to rely on it being present in some way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

2021-06-11 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri created this revision.
lebedev.ri added reviewers: thopre, bjope, nikic, chandlerc, aeubanks.
lebedev.ri added a project: LLVM.
Herald added subscribers: ormris, wenlei, steven_wu, hiraditya.
lebedev.ri requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Addition of this pass has been botchered.
There is no particular reason why it had to be sold as an inseparable part of 
new-pm transition.
It was added when old-pm was still the default, and very *very* few users were 
actually tracking new-pm,
so it's effects weren't measured. Which means, some of the turnoil of the 
new-pm transition
are actually likely regressions due to this pass.

Likewise, there has been a number of post-commit feedback (post new-pm switch), 
namely

- https://reviews.llvm.org/D37467#2787157 (regresses HW-loops)
- https://reviews.llvm.org/D37467#2787259 (should not be in middle-end, should 
run after LSR, not before)
- https://reviews.llvm.org/D95789 ()

and in the half year past, the pass authors (google) still haven't found time 
to respond to any of that.

Hereby it is proposed to backout the patch from the pipeline, until someone who 
cares about it
can address the issues reported, and properly start the process
of adding a new pass into the pipeline, with proper performance evaluation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104099

Files:
  clang/test/CodeGen/thinlto-distributed-newpm.ll
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll


Index: llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
@@ -199,7 +199,6 @@
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
Index: llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
@@ -187,7 +187,6 @@
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -218,7 +218,6 @@
 ; CHECK-POSTLINK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-POSTLINK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-POSTLINK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: ConstantMergePass
Index: llvm/test/Other/new-pm-defaults.ll
===
--- llvm/test/Other/new-pm-defaults.ll
+++ llvm/test/Other/new-pm-defaults.ll
@@ -237,7 +237,6 @@
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-EP-OPTIMIZER-LAST: Running pass: NoOpFunctionPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -1443,11 +1443,6 @@
   // resulted in single-entry-single-exit or empty blocks. Clean up the CFG.
   OptimizePM.addPass(SimplifyCFGPass());
 
-  // Optimize PHIs by speculating around them when profitable. Note that this
-  // pass needs to be run after any PRE or similar pass as it is essentially
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-
   if (PTO.Coroutines)
 OptimizePM.addPass(CoroCleanupPass());
 
Index: clang/test/CodeGen/thinlto-distributed-newpm.ll
===
--- clang/test/CodeGen/thinlto-distributed-newpm.ll