[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Zebediah Figura]

On 10/14/21 12:52 PM, Giovanni Mascellani wrote:
> Hi,
> 
> Il 14/10/21 18:32, Zebediah Figura ha scritto:
>> Yeah, but the ternary operator is a special case. I don't think it
>> should result in an HLSL_IR_EXPR. Probably we should synthesize a
>> temporary variable, store to it inside of a conditional, and load from it.
> 
> Why? I don't know in SM1, but at least in SM4 we have a nice opcode
> (MOVC) that does just what the ternary operator needs to do, in a way
> that is probably more efficient (or at least not less efficient) than
> doing an actual branch. Also, I have a few other patches in my queue
> that use MOVC for other things (like implementing casts from bool), so
> I'd like to have it in the IR (not really related to this patch, though).

Well, the usual answer, which is that keeping the scope of the IR low 
makes it easier to work with.

Maybe there's an argument for having a ternary/movc in the IR, but even 
if there is, I'm not sure we really want to make add_expr() more complex 
just for that. Are there any other expressions that (might) take 
operands of non-uniform types?

> 
>> I'm still not convinced that casting in add_expr() is better than
>> casting in the caller, either.
> 
>   From my point of view it makes the code more declarative and therefore
> easier to read. However apparently we have different tastes, and I don't
> have a too strong opinion.

I guess it's not clear to me that it's actually making the caller's job 
easier, and in lieu of that I'm inclined to prefer modularity. Not to 
get too philosophical, but while I'm a fan of declarativity, I tend to 
shy away from giving a function too many parameters if I can avoid it.

> 
>>> The add_*_last helpers behave similarly as their counterpart without
>>> "_last", except that the last node in each instruction list is used
>>> as an argument.
>>
>> Yeah, I still don't like this name. "last" makes me think that the
>> instruction will be added to the end of a list, or something. "merge" or
>> "combine" would be a lot better, but it's a bit awkward for the unary
>> helpers. But then again maybe we don't need the unary helpers, and we
>> can just as easily just spell out node_from_list() inline. There are
>> only three such cases after all (and I guess they're not even all the
>> same kind of operation).
>>
>> Or "parse", I guess, to symbolize it's done as part of parsing, although
>> that's not great either.
> 
> I don't like it either, but neither I like better one of your
> alternatives, nor I could find anything better. Among your suggestions I
> would pick "parse", but if you eventually settle for something else just
> tell me.

I would prefer "merge" if we can find a way to avoid needing it for 
unary helpers. Does that latter part seem too unpalatable?

> 
>> I think at least splitting the "cast_types" part out of this patch would
>> make it easier to read.
> 
> Ok, will do.
> 
> Thanks again, Giovanni.
>