[PATCH vkd3d 2/5] vkd3d-shader/hlsl: Perform a copy propagation pass.

[Giovanni Mascellani]

Hi,

On 10/11/21 17:33, Zebediah Figura (she/her) wrote:
>> Mmh, note that you have to handle control flow in some way or another, 
>> you cannot just ignore it. This patch doesn't know about control flow, 
>> but the only way it can do it is by bailing out at the first control 
>> flow instruction in the program. It's not enough to ignore the control 
>> flow block and resume processing after it, because the inner block 
>> might invalidate some of your knowledge about the program state (in 
>> 3/5 patch this is done by copy_propagation_invalidate_from_block).
> 
> Right. I should have said to invalidate the whole copyprop state both 
> when entering a CF block and when leaving one. The point is that we can 
> do copyprop within every BB, not just the first one, without having to 
> worry about things like partial invalidation.

I guess that by now I've already read the other mail I sent on that, but 
the point is that wouldn't be enough to get texture working, I think.

>> Personally I would have loved to meet comments like mine in other 
>> areas of Wine: knowing what data read or written by an algorithm are 
>> meant to represent makes it much easier to understand why the 
>> algorithm does what it does. I agree that this is not the most 
>> complicated case around, but I thought this would be helpful for 
>> someone not already knowing my code. OTOH I understand that also a 
>> high-level description is useful: would you consider it satisfying to 
>> have both?
> 
> Eh, maybe. I would at least start with the high-level explanation. When 
> I read the above comment my eyes kind of glaze over, and it doesn't tell 
> me anything that the code already doesn't (even the struct definitions 
> alone tell me as much without checking how they're used.)
> 
> I dunno, I'll see what you end up coming up with.

Ok, I'll do my best.

> Er, sorry, I should have said intptr_t. I mean to return intptr_t from 
> the rbtree compare callback, and then all you need is
> 
>      return (intptr_t)key - (intptr_t) variable->var;

Would that really work? For one thing, signed overflow is undefined 
behavior in C (except possibly very recent versions which we're 
definitely not using). It doesn't seem we're using -fwrapv, does it?

Even assuming two's complement semantics, this doesn't seem to give an 
order: for example, 0 > (INT_MIN + 1), because 0 - (INT_MIN + 1) = 
INT_MAX > 0 and INT_MAX > 0, because INT_MAX - 0 = INT_MAX > 0, but 
(INT_MIN + 1) - INT_MAX = 2 > 0, therefore you'd have (INT_MIN + 1) > 
INT_MAX. Therefore the relationship is not transitive, which I am pretty 
sure the red-black tree implementation won't like.

Am I missing something?

> Ah. I assumed that you would actually be indexing right before copyprop, 
> and didn't check whether that was actually the case.
> 
> Although on reflection now I'm not sure if we *should* index. We only 
> dump the IR once, at the end, and I feel like we risk confusion by 
> printing instruction indices that won't match. Hmm, maybe the code makes 
> sense as it is...

You're right, I'd say at this point it's better to just ignore the index 
and always use the pointer, so there is no confusion with the indices in 
the final dump. This also means that I can put the %p directly in the 
TRACE without the additional buffer that you didn't like (and removing 
like 10 lines of code used just for tracing).

> I'm typically in favor of leaving in traces if they were helpful during 
> debugging.
> 
> For what it's worth, I'd also be in favor of a "debugstr_node" helper or 
> something. Maybe even if it uses a fixed-size buffer, although I'm not 
> sure that Matteo or Henri will agree with that ;-)

If you accept my solution above, this has just been swept away.

>>> Shouldn't this function just return void?
>>
>> It's a matter of philosophy. If you ask me, the switch in 
>> copy_propagation_recursive should be rather oblivious of what the 
>> various helpers actually do, it shouldn't have an insight that 
>> copy_propagation_store never returns true. For its point of view, any 
>> helper can potentially modify the code. Though I can change to void if 
>> you prefer.
> 
> It's a fair point, but I guess I look at copy_propagation_recursive() 
> and think "recording stores should never make any progress; all the 
> magic happens in rewriting loads". This becomes especially true if this 
> function gets renamed to copy_prop_record_store() or something, which I 
> think it should.

Ok, you win on this one.

Thanks again, Giovanni.