[llvm-commits] [PATCH] Allow converting ConstantExprs to Instructions

[James Molloy]

Hi Renato,

> A ConstantExpr *is* a Value,

Indeed, this was poor wording on my part.

As ConstantExpr is a Constant, its operands must be Constants. And it itself can be used as an operand to something expecting a Constant.

If I wish to change it or one of its operands to an Instruction, I cannot do that (because, obviously, Instructions aren't constant). There is no easy way to go from a ConstantExpr to its Instruction corollary, and that seems an unnecessary restriction.

That is why I'm adding this extra API function. The function I added does *not* bypass the hierarchy, IMHO, as every ConstantExpr has an Instruction counterpart.

Where I said "Value" before, I meant "Value which is not a subclass of Constant".

> Have you thought about adding constructors for the Instructions
> passing ConstantExpr references? That might be used automatically by
> the compiler as a casting match, which will be even neater.

That seems like a heavyweight solution, and I also disagree with it because the current implementation as sent keeps all the implementation details of ConstantExpr (of which there are many) located and isolated in the same .cpp file. Adding implicit constructors which take a ConstantExpr would spread implementation details of ConstantExpr around the codebase.

Also, Richard Smith mentioned at the dev conference that there may be a push to remove ConstantExpr completely, and if so my current implementation would be much easier to clean up :)

Cheers,

James
______________________________________
From: rengolin at gmail.com [rengolin at gmail.com] On Behalf Of Renato Golin [rengolin at systemcall.org]
Sent: 16 November 2012 20:23
To: James Molloy
Cc: llvm-commits at cs.uiuc.edu; cfe-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Allow converting ConstantExprs to Instructions

On 16 November 2012 18:17, James Molloy <James.Molloy at arm.com> wrote:
> The attached patch implements a simple extra API function to retrieve an
> Instruction* equivalent for a ConstantExpr. With this addition, dealing
> with the constraints imposed by SPIR is actually possible without
> horrible hackery...

A ConstantExpr *is* a Value, so the wrapper you did bypasses the
hierarchy (possibly broken), but is also disconnected from the API,
which means when either side change, you might forget to change this
bridge and get away with it.

Having said that, I don't think the consequences of forgetting to
update this code is drastic (I can't see why it'd do anything other
than assert at compile time), but since there is already a connection
between ConstantExpr and Value, I think it's a cheap trick.

Have you thought about adding constructors for the Instructions
passing ConstantExpr references? That might be used automatically by
the compiler as a casting match, which will be even neater.

--
cheers,
--renato

http://systemcall.org/