> Oh, I was looking for a rationale for the restriction to constant-ness > (and specifically - off my head, without rechecking the patch - data > structure limitations from that decision). So yes, we can restrict > to constants, but can we keep the infrastructure in a way that we > _could_ allow non-constants? Like maybe SSA default defs > (parameter values)?
I need to think about that. The way it is structured right now is relatively constant centric. We could replace the constant factors in the hash map with SSA names and adjust the descent routines but costing would certainly become more difficult. What's not yet clear to me yet is how complex mixed trees would become then. With just constants, multiplications are always simple factors and everything stays nice and linear. But for a * (b + (c + (d * e))) or (a + b) * (c + d) I find it much less intuitive what to reasonably do. > To add, assuming after gathering the ops list and performing some > simplification, we might want to try hard to recover parts in the result > that had been computed in the original IL. Consider > > ((a + b) + (d - c) + c) > > we want > > (a + b) + d > > and not, say, > > (int)(((unsigned)a + (unsigned)d) + (unsigned)b) Ah yes, I see. I was actually hesitant to go there at all yet (unsigned rewriting) and the signed handling in the series right now is just (global) range based. Therefore it won't touch the more interesting or more involved cases. Once rewriting is added, I was thinking of costing an unsigned conversion somehow (not "preprocessing" trees with too many of them) but didn't converge on a good idea yet. > Note I only looked at parts of the series, so in case you have no > changes pending tell me and I'll actually review the whole thing. > Otherwise I'll wait for a v2. I don't have pending local changes apart from typo fixes and moving of hunks (like constant_maybe_expensive which is unused in patch 1). Maybe, if you get to it, patch 1 and 2 would be best, as they establish the baseline. The others are mostly straightforward extensions and would need adjustments anyway if 1 or 2 are controversial. FWIW Patch 2 wouldn't change a lot when being split out. -- Regards Robin
