[PATCH] D59227: [GlobalISel][Utils] Teach getConstantVRegVal how to look through trunc and z|sext

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Disclaimer: this is just my opinion and in particular I haven't talked with anybody about constants being place holders. So just see my comment as what they are: an individual throwing ideas :).

> This isn't how I think of G_CONSTANT. I think of it as a mov setting a legal result register that is a legitimate instruction in its own right and not just a placeholder. The immediate folding rules for AMDGPU are somewhat complex, so an arbitrary constant is always going to be selected as a materializing move. We wouldn't want G_* instructions to always fold constant operands since that would create more work.

I agree that the G_CONSTANT are a mov to a legal result register. However, I don't think they should be legalized on there own. Essentially, if we truncate/extend the constants directly during the legalization of their uses, they naturally become legal (the illegal ones become dead). I.e., we don't have to handle G_CONSTANT explicitly. This is true only if the builders don't create thing like ext(cst), like they do today, because then we indeed have to legalize those immediate moves.

To go back to constant selecting to thing that could be expansive, the fact that we decide to model them as instructions was also to account for that. However, the way we effectively deal with them is not what I had in mind:

- A given constant should appear only once (this is workaround with the CSE builder right now, but that's not how I envisioned thing)
- Constant should be selected last: at this point we would see if they have any users and thus if they need to be materialized into a register

The second point was supposed to come from free as in my mind constants would have been uniquely represented and placed in the entry block, which is proceeded last (i.e., all users have been seen).

Anyway, I am not planning on changing that anytime soon (if ever at all) and because of that this patch is unfortunately required. Bottom line, this patch is a workaround a more deeper problem in the way we handle constants IMHO.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59227