[AMDGPU] Optimize image sample followed by llvm.amdgcn.cvt.pkrtz into d16 variant #145203
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,6 +247,42 @@ simplifyAMDGCNImageIntrinsic(const GCNSubtarget *ST, | |
| ArgTys[0] = User->getType(); | ||
| }); | ||
| } | ||
|
|
||
| // Fold image.sample + cvt.pkrtz -> extractelement idx0 into a single | ||
| // d16 image sample. | ||
| // Pattern to match: | ||
| // %sample = call float @llvm.amdgcn.image.sample... | ||
| // %pack = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %sample, | ||
| // float %any) | ||
| // %low = extractelement <2 x half> %pack, i64 0 | ||
| // Replacement: | ||
| // call half @llvm.amdgcn.image.sample | ||
|
Comment on lines
+253
to
+259
There was a problem hiding this comment. This seems OK, but do we need to handle more cases? E.g.
Incidentally it would easier to implement the pattern matching if we provided a scalar There was a problem hiding this comment. Thanks, Jay. I’ve thought about the cases you mentioned.
Did you mean something like this? define amdgpu_ps float @image_sample_2d_single_pkrtz_two_sample_no_d16(<8 x i32> %surf_desc, <4 x i32> %samp, float %u, float %v) {
entry:
%sample1 = call float @llvm.amdgcn.image.sample.lz.2d.f32.f32.v8i32.v4i32(i32 2, float %u, float %v, <8 x i32> %surf_desc, <4 x i32> %samp, i1 false, i32 0, i32 0)
%sample2 = call float @llvm.amdgcn.image.sample.lz.2d.f32.f32.v8i32.v4i32(i32 2, float %u, float %v, <8 x i32> %surf_desc, <4 x i32> %samp, i1 false, i32 0, i32 0)
%pack = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %sample1, float %sample2)
%h0 = extractelement <2 x half> %pack, i64 0
%h1 = extractelement <2 x half> %pack, i64 1
%mul = fmul half %h0, %h1
%div = fdiv half %mul, %h0
%add = fadd half %div, %h1
%res = fpext half %add to float
ret float %res
}But in fact LLPC only uses the low half, you can find the details in LLPC: m_builder->CreateFpTruncWithRounding(inst->getOperand(0),.....so the second operand is usually a constant 0.0.
It’s a similar case. Only the first result of each pkrtz is used. For example: define amdgpu_ps float @image_sample_2d_single_pkrtz_d16(<8 x i32> %surf_desc, <4 x i32> %samp, i32 %u, i32 %v) {
entry:
%0 = call reassoc arcp contract afn <4 x float> @llvm.amdgcn.image.load.2d.v4f32.i32.v8i32(i32 15, i32 %u, i32 %v, <8 x i32> %surf_desc, i32 0, i32 0)
%1 = extractelement <4 x float> %0, i64 3
%2 = extractelement <4 x float> %0, i64 2
%3 = extractelement <4 x float> %0, i64 1
%4 = extractelement <4 x float> %0, i64 0
%5 = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %4, float 0.000000e+00)
%6 = extractelement <2 x half> %5, i64 0
%7 = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %3, float 0.000000e+00)
%8 = extractelement <2 x half> %7, i64 0
%9 = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %2, float 0.000000e+00)
%10 = extractelement <2 x half> %9, i64 0
%11 = call <2 x half> @llvm.amdgcn.cvt.pkrtz(float %1, float 0.000000e+00)
%12 = extractelement <2 x half> %11, i64 0
%mul1 = fmul reassoc arcp contract afn half %6, %8
%mul2 = fmul reassoc arcp contract afn half %10, %12
%add = fadd reassoc arcp contract afn half %mul1, %mul2
%res = fpext half %add to float
ret float %res
}I plan to support this case in a follow-up patch after some refactoring: :#145312 (comment). Do we have a There was a problem hiding this comment. |
||
| // | ||
| // Folding criteria: | ||
| // 1. The only user of the image.sample intrinsic is amdgcn.cvt.pkrtz. | ||
| // 2. That cvt.pkrtz call has exactly one use. | ||
| // 3. Its sole user is an extractelement instruction with index zero. | ||
| // Otherwise, folding is not performed, because D16 sampling only | ||
| // guarantees that the element at index 0 is defined; index 1 is | ||
| // undefined and using it will result in poison. | ||
| if (auto *CvtPkrtzCall = dyn_cast<CallInst>(User)) { | ||
| if (CvtPkrtzCall->getIntrinsicID() == Intrinsic::amdgcn_cvt_pkrtz && | ||
| CvtPkrtzCall->hasOneUse()) { | ||
| // Unique use must be extractelement idx == 0 | ||
| if (auto *Ext = | ||
| dyn_cast<ExtractElementInst>(*CvtPkrtzCall->user_begin())) { | ||
| if (isa<ConstantInt>(Ext->getIndexOperand()) && | ||
| cast<ConstantInt>(Ext->getIndexOperand())->isZero()) { | ||
|
|
||
| return modifyIntrinsicCall( | ||
| II, *CvtPkrtzCall, ImageDimIntr->Intr, IC, | ||
| [&](auto &Args, auto &ArgTys) { | ||
| ArgTys[0] = CvtPkrtzCall->getType(); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Only perform D16 folding if every user of the image sample is | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a similar optimization in mesa and it broke tests because D16 does not round the same as a 32bit load followed by v_cvt_pk_f16_f32 for fixed point formats. D16 directly rounds towards nearest even in fp16, while the unoptimized pattern will first round towards nearest even in fp32, and then rounds towards zero in fp16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Could you share some test cases for me to verify this issue? So far, I haven’t encountered the problem you mentioned. Since image sampling with D16 only uses the lower 16 bits, and I ensure that the first argument to cvt.pkrtz is the result of the image sample, I believe this folding is valid. I’ve tested a few cases and haven’t observed any issues so far, but I’ll continue testing more to be safe.
Additionally, we've used this optimization in other compilers (not LLVM) for years without encountering any problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mesa change was in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28730, but sadly the CI pipeline data was already deleted, so finding the exact test case that failed back then isn't trivial. But it resulted in disabling D16 optimizations if a fp16 rounding mode is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will test it again. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this optimization is not OK unless we have some extra information saying that it's OK to use a different rounding mode. (But then if it's OK to use the normal round-to-nearest-even mode then why would you generate pkrtz instructions in the first place?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you mean that if the input data is in fixed-point format, it gets converted to fp32 using RTE, and then if we use pkrtz, it applies RTZ?
If we fold this into a D16 image sample, it will only apply RTZ, which could cause accuracy issues. Is that what you're saying?
But in the IR I can't see any data format info in the image instruction, and I haven’t found a case that clearly uses a fixed-point format.
Do you have an example? I'd like to verify it. By fixed-point formats, you mean things like UNORM, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DadSchoorse This PR will impact mesa? right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not know the format. It's a potentially runtime value in the resource descriptor. You do not have enough information to perform this transformation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I’m not sure how to achieve this format yet , I’m still thinking about the issue. Would it be possible to make this optimization optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For image_load and image_sample instructions specifically, there is no conversion from int to float or float to int. The result is int or float based on the in-memory type. So do you think there are still cases that this patch would handle incorrectly @DadSchoorse ?