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

Tue Feb 16 20:15:44 PST 2016

Reading over the current code, I still dislike the new interface.  I 
find it obscures what's actually happening and is IMO clearly worse than 
the original code.  I'm not as strongly opposed as Danny seems to be, 
but the fact Danny is so opposed is itself a worrying sign. :)

Another approach might be to redefine what the DestTy operand of the 
original API meant.  We have several instructions which need an 
additional type specifier (i.e. the types of the operands are not 
sufficient).  For loads, stores, and casts, this type specifier happens 
to be the result type.  For GEPs, it could be defined as the indexing 
type.  We would need to introduce an accessors to make getting this type 
specifier easily accessible, but it would leave much of the interface 
intact.

Philip

On 02/09/2016 03:05 PM, Manuel Jacob via llvm-commits wrote:
> On 2016-01-26 07:20, Daniel Berlin wrote:
>> On Mon, Jan 25, 2016 at 7:04 PM, Manuel Jacob <me at manueljacob.de> 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.\
>>>
>>
>> It happens :)
>>
>>
>>>
>>> > 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.
>>>
>>>
>> But it's also not possible for comparisons, so it's just "another thing
>> that it doesn't work on". That's fine :)
>>
>>> Additionally, i can't see how use cases like mine are supposed to
>>> function - i have what was an instruction, but is now an operation 
>>> and some
>>> operands.
>>>
>>> >  I want to see if it will fold to a constant.
>>>
>>> >
>>>
>>> > What API am i expected to use to do so?
>>>
>>> >
>>>
>>> > Note also that the InstOperands API was *explicitly* designed to not
>>> require an instruction, to support the above use case. This was the
>>> difference between ConstantFoldInst and ConstantFoldInstOperands.  It
>>> happens most users can pass it instructions, but that doesn't , IMHO 
>>> make a
>>> lot of sense.
>>>
>>>
>>> ConstantFoldInstOperands constant folds the given instruction **but 
>>> with
>>> the specified operands**. If I understand correctly, you still have the
>>> instruction available.
>>>
>>>
>> I do *not* have the instruction available.
>> I have it's opcode, operands, type, etc.
>> (For geps, i have the source element type as well ).
>>  But i don't have the original instruction, and in fact, there isn't 
>> alwas
>> a mapping
>
> Looking at commit 9a08577cb28fabfb4aa8509f73fc4411aba1f81b in your 
> gvn-rewrite fork, adapting your code seemed to be straightforward.  
> I'm happy to re-add the form taking opcode and result type to not 
> hinder your ongoing work, but please don't complain if I restart the 
> discussion after you have upstreamed the new GVN pass. ;)
>
>>> I want to request that you provide some way to not require the
>>> instruction, as the current API does (and many users use!), so that 
>>> folks
>>> can use this without creating fake instructions to do so.
>>>
>>>
>>> The problem with such an API is that it doesn't work with getelementptr
>>> (and not with comparison instructions, as it was not possible before 
>>> this
>>> patch). I think that most use cases are covered by already existing
>>> functions.
>>>
>>>
>> Sure, but constantfoldinstoperands handled dispatching to those 
>> functions,
>> otherwise, you have to write and update a dispatch function constantly.
>>
>> 1. The user wants to fold an instruction but with other operands (e.g.
>>> because they were constant-folded by the user). 
>>> ConstantFoldInstOperands
>>> should work here.
>>
>> What if i want to fold a generic operation without having to worry about
>> whether it is specifically a binary op, cast, etc.
>>
>> You've basically pushed all that logic/work into the callers, where 
>> it will
>> be duplicated.
>
> This is exactly what I wanted to avoid with this change.  Instead of 
> pushing the checks for GEPs to the caller (as it's already necessary 
> for comparisons) I changed the interface to take an instruction, which 
> made sense for all in-tree users, and I have not yet seen out-of-tree 
> code where this is currently a problem.
>
> -Manuel
>
>> 2. The user wants to fold a binary operation or cast. I added 
>> functions for
>>> this in http://reviews.llvm.org/rL258389 and
>>> http://reviews.llvm.org/rL258390.
>>>
>>> There is no API for the case when the user has a random instruction 
>>> opcode
>>> but no instruction. I don't think that adding a function for this case
>>> which however doesn't work with getelementptr and compares is very 
>>> helpful
>>> in this case.
>>>
>>
>> I'm going to pretty strongly disagree. Not everything in LLVM 
>> necessarily
>> has an instruction associated with it yet.
>>
>> Things like PRE, etc, often have a bunch of operands an opcode, and 
>> want to
>> know what will happen. Before you rpatch, this was easy.
>>
>> Now, it's literally impossible to get *any* results (instead of just 
>> "give
>> me the results you can").
>>
>> That seems like a pretty bad API regression.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits