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

Fri Mar 4 18:17:50 PST 2016

On 2016-02-17 05:15, Philip Reames wrote:
> 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. :)

I agree that the old interface was a bit more obvious than the form I 
changed it to (at least at the first sight).  But looking at the users 
(in-tree at least) of this interface, the new interface seems to be a 
better fit.  The use case for this interface is that there is an 
instruction and the operands were already constant-folded or otherwise 
simplified.  The name and documentation could be changed to make it 
clearer, though.

The new interface also enables a potential change where the function 
also accepts loads and compares.  This makes it possible to further 
simplify the callers, which currently have to check whether the 
instruction is a load or compare.

It is true that I removed the functionality to constant-fold a given 
opcode, return type and operands.  I haven't seen a single user of this 
function where the instruction wasn't available or the other functions 
in the constant-folding API couldn't be used (to be fair, I added 
ConstantFoldBinaryOpOperands and ConstantFoldCastOperand prior to this 
change).  I don't think we should design an API around a hypothetical 
use case.  The previous form couldn't take load and compare opcodes (and 
now GEPs), so in any case callers couldn't pass arbitrary opcodes to 
this function.

> 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.

(Minor comment: in an opaque pointer future, the characterizing type of 
a store will be the type of the value to be stored.)

Do we agree that just redefining what the DestTy parameter of the 
function means is a bad idea because it would break existing code in a 
non-obvious way?  If the answer is yes, how would the interface you 
propose look like?

-Manuel

> 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