[PATCH] D16383: Change ConstantFoldInstOperands to take Instruction instead of opcode and type. NFC.

[Daniel Berlin via llvm-commits]

On Tue, Jan 26, 2016 at 4:51 PM, Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> On 01/25/2016 07:04 PM, Manuel Jacob via llvm-commits wrote:
>
>> mjacob added a comment.
>>
>> In http://reviews.llvm.org/D16383#335626, @dberlin wrote:
>>
>> Hi Manuel,
>>>
>>> Things like NewGVN don't always have instruction *'s to hand to
>>> ConstantFoldInstOperands.
>>>
>>> For example, we create an expression that stores operands and opcodes
>>> and such (like GVN does!)
>>>
>>> Before, we could pass the opcode and operands, and that worked great.
>>>
>>> Now you've made it take the Instruction.
>>>
>>
>> Sorry for making difficulties.
>>
>> I don't see how this makes anything related to opaque pointers easier,
>>> maybe you could enlighten me?
>>>
>>
>> The problem is getelementptr. With opaque pointers, we must have the
>> instruction to get the source element type. Currently this is possible by
>> getting the pointee type of the first (pointer) operand of the GEP. This
>> won't be possibly with opaque pointers.
>>
> Why is this not as simple as adding a type parameter to the existing API?
>
>>
>>
What he said.  As I said, without more info, it's pretty  unclear to me why
this is the right design compared to what was there before.

Looking at this set of changes, you pass the instruction later just to get
the opcode again and pass it to an internal helper. This whole thing seems
a bit odd compared to just passing along the type and being done with it.

(In a later set of patches, this is pushed down even one more time, so you
pass a value *, then try to get the opcode from it again, and .... Again,
this all seems much worse than just passing the opcode down as it was now,
and much uglier.  You are passing a bunch of 8 byte value *'s just for a 16
bit piece of data they happen to have stored somewhere so you can branch on
it, when the old one just passed the 16 bits of data it actually wanted)

I also don't see any discussion of the API changes you were going to make
(which seem to be not small) at all on llvmdev, or i would have pointed
this out earlier for you and we could have had this discussion then :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160126/bb4c990f/attachment.html>