[Downstream Change] Revert ccef34 and related commits - #966
[Downstream Change] Revert ccef34 and related commits#966john-brawn-arm wants to merge 1 commit into
Conversation
|
This pull review modifies files outside of the |
|
What is the performance impact on hpcapps of this change? I doubt there isn't any. |
dcandler
left a comment
dcandler
left a comment
There was a problem hiding this comment.
I can give an approval from the ATfE team since we agreed that it was necessary to fix the regression for the release, but do please get approval from the ATfL team as well if it can potentially impact their performance too.
david-arm
left a comment
david-arm
left a comment
There was a problem hiding this comment.
Do we have any performance data showing the impact of these reverts?
| ; CHECK-NEXT: [[WIDE_MASKED_LOAD6:%.*]] = call <4 x double> @llvm.masked.load.v4f64.p0(ptr align 8 [[TMP9]], <4 x i1> [[REVERSE5]], <4 x double> poison) | ||
| ; CHECK-NEXT: [[TMP10:%.*]] = fadd <4 x double> [[WIDE_MASKED_LOAD]], splat (double 1.000000e+00) | ||
| ; CHECK-NEXT: [[TMP11:%.*]] = fadd <4 x double> [[WIDE_MASKED_LOAD6]], splat (double 1.000000e+00) | ||
| ; CHECK-NEXT: [[REVERSE6:%.*]] = shufflevector <4 x double> [[WIDE_MASKED_LOAD]], <4 x double> poison, <4 x i32> <i32 3, i32 2, i32 1, i32 0> |
There was a problem hiding this comment.
It's unfortunate that we're removing these folds, however this is more of a cost model problem since the instcombine pass would apply the same folds afterwards. Removing the shuffles or vector.reverse calls increases the likelihood of vectorisation by lowering the cost.
You can see the effects of this in the vector-reverse.ll test below. I find it hard to believe this doesn't introduce a regression somewhere, but if these reverts have been fully tested on various benchmarks then I guess it's ok.
Running hpcapps on graviton3 it looks like there are some regressions, some improvements, and the geomean score is better:
|
|
Thanks @john-brawn-arm for these figures. I think we need to ask @kiranchandramohan for the final say. |
|
Looking at this PR and (comments in) #984 we can clearly see a conflict of (performance) interest between ATfE and ATfL. Considering we plan to introduce more cherry-picks like that, I think we need to call an emergency meeting on how can we immediately decouple ATfL and ATfE repositories to avoid problems in this and any future releases. |
|
Any movement on the decision for this? |
6 participants
This is being done to fix a regression in an important embedded benchmark for ATfE 23.
This reverts the following commits:
ca970f3
a47be19.
024a691
ccef34d
Downstream issue: #965