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

[Daniel Berlin via llvm-commits]

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

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



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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160125/6ca634bf/attachment.html>