GLSL: Add shifts and bit ops to sensitive operation list.#2633
Conversation
| case OpSampledImage: | ||
| case OpImage: | ||
| // OpCopyObject intentionally excluded. | ||
| case OpCopyObject: |
There was a problem hiding this comment.
Why is this being reintroduced now where the previous commit didn't? The way I understand it, the root cause is that shifts and bitops weren't subject to precision.
There was a problem hiding this comment.
There are no changes on this line. You are looking at the commit that reverted my previous changes. If you look at the final changes the only difference is the addition of the OpShift and OpBit instructions to the sensitive operation list.
There was a problem hiding this comment.
Ok, I see you came to the conclusion in the issue. I review commit by commit rather than everything together, so that's why I got confused.
| @@ -0,0 +1,74 @@ | |||
| ; Verifies that OpCopyObject's result precision is determined by its OWN | |||
| ; RelaxedPrecision decoration, not by the operand's. The SPIR-V spec defines | |||
| ; OpCopyObject as: "Make a copy of Operand. There are no pointer dereferences | |||
There was a problem hiding this comment.
I'm not sure it's correct to assume OpCopyObject would work like an "expanding" operation if it's not marked with relaxed precision.
E.g. a pattern of:
OpDecorate %narrow Relaxed
%narrow = OpSomeOp
%copy = OpCopyObject %narrow
%shift = OpShiftOp ... %copy
If %shift is not marked relaxed, I think the implication is that the operation happens at full precision, which means only at that point the relaxed input gets expanded. And that's what I understand this PR to do since I missed the cases of integer bitops.
There was a problem hiding this comment.
You are commenting on my previous attempt at solving this problem, which was to insert OpCopyObject instructions to narrow or widen the variables. I was coming at this problem thinking it was a spirv-opt issue, since the problem was introduced during ssa-rewrite and did not occur otherwise. I thought I could solve it with the insertion of the explicit copies, but was surprised to find spirv-cross forwarded the precision status to the destination defeating my attempts to address this. After taking a closer look at why spirv-cross behaved that way I realized you already had the machinery to handle this correctly, and that the only missing element was the addition of the shifts and bit ops to the precision-sensitive operation list, which is what this PR is proposing.
2 participants
Another attempt at fixing #2630. Restore the behavior of
OpCopyObjectas a precision forwarding instruction and instead ensure shifts and bit opts are performed at the expected precision by adding them to the sensitive operation list so that they take theanalyze_precision_requirementscode path.