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

[Manuel Jacob via llvm-commits]

On 2016-01-27 01:57, Daniel Berlin wrote:
> 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)

Sorry, but it seems like you have understood the code wrong.  The new 
code gets the opcode from the instruction once.  The instruction is 
passed to the ConstantFoldInstOperandsImpl helper as parameter 
`InstOrCE` and used to get the type and, in case of a GEP instruction, 
the source element type (this extra parameter was added in a follow-up 
commit though, as it's part of the ongoing work to make pointers 
opaque).  Otherwise the ConstantFoldInstOperandsImpl function is very 
similar to the previous ConstantFoldInstOperands function, taking opcode 
and type.  I'm happy to (re-)add a public function taking opcode and 
type if you think it is useful.  However this function won't work with 
GEPs.

I think all code I've seen using the previous interface (all in-tree 
users and the code in your gvn-rewrite fork) had an instruction 
available and passed both its opcode and type to the function.  Passing 
just the instruction made sense for these users.  Although I didn't see 
any user which doesn't have the instruction available and can't use the 
other constant folding API without adding special cases, I'll of course 
trust your judgement that such a use case is possible.

> 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 :)

My impression is that API changes are common in LLVM and, unless it's 
about removing target hooks, usually not discussed on llvm-dev in 
addition to the normal review process.  That said, we can of course 
discuss it on llvm-dev if we don't come to a quick conclusion here.

-Manuel