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

[Manuel Jacob via llvm-commits]

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.