<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 26, 2016 at 4:51 PM, Philip Reames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
On 01/25/2016 07:04 PM, Manuel Jacob via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
mjacob added a comment.<br>
<br>
In <a href="http://reviews.llvm.org/D16383#335626" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16383#335626</a>, @dberlin wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Manuel,<br>
<br>
Things like NewGVN don't always have instruction *'s to hand to ConstantFoldInstOperands.<br>
<br>
For example, we create an expression that stores operands and opcodes and such (like GVN does!)<br>
<br>
Before, we could pass the opcode and operands, and that worked great.<br>
<br>
Now you've made it take the Instruction.<br>
</blockquote>
<br>
Sorry for making difficulties.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I don't see how this makes anything related to opaque pointers easier, maybe you could enlighten me?<br>
</blockquote>
<br>
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.<br>
</blockquote></span>
Why is this not as simple as adding a type parameter to the existing API?<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br></blockquote></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>(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)</div><div><br></div><div>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 :)</div><div><br></div><div> </div></div></div></div>