rsmith added a comment.

In D85710#2212174 <https://reviews.llvm.org/D85710#2212174>, @rjmccall wrote:

> I always imagined that we'd implement this by just inverting the emission: 
> emit the RHS of the assignment and then pass a callback down to a function 
> that descended into conditional operators and called the callback when it hit 
> a leaf.  Having an LValue case that's an arbitrarily-nested conditional seems 
> unfortunate.

I thought I'd convinced myself that that approach couldn't work, but I'm no 
longer so convinced. (I had thought there were cases where we would need to run 
arbitrary code between creating the lvalue and disposing of it, and that 
arbitrary code could be something we were only able to emit once for whatever 
reason.) The fact that assignment and compound-assignment operators all 
evaluate their RHS before their LHS (in the language modes where they're 
sequenced) helps a lot; expressions such as `((cond ? a.x : a.y) += f()) *= 
g()` still seem like they could compositionally work with this model, and even 
things like `(cond1 ? b : (cond2 ? a.x : a.y) += f()) += g()`. Hm, but what 
about `int n = (cond ? a.x : a.y) += 1;`? I guess we'd need to carry the intent 
to perform a load of the conditional lvalue into the callback too.

OK, I think that all works, but I'm a lot less confident of the absence of 
surprising corners that would defeat that strategy than with the approach of 
this patch -- the correctness of doing this with a single branch seems to hinge 
crucially on subtle program structure restrictions, in particular the 
presumptive fact that we never need control flow to converge between creating 
the lvalue and consuming it. On the other hand, it would also let us emit 
better IR for things like `(a += 1) *= 2` (removing the intermediate store to 
`a`) if we wanted, and maybe it's a good strategy to pursue for that reason?

I can give that a go next time I find some cycles to work on this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85710/new/

https://reviews.llvm.org/D85710

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D85710: C... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D857... Richard Smith - zygoloid via Phabricator via cfe-commits
    • [PATCH] D857... John McCall via Phabricator via cfe-commits
    • [PATCH] D857... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to