<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On May 30, 2008, at 5:58 PM, Dan Gohman wrote:</div><blockquote type="cite"><div>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=51806&view=rev">http://llvm.org/viewvc/llvm-project?rev=51806&view=rev</a><br>Log:<br>IR, bitcode reader, bitcode writer, and asmparser changes to<br>insertvalue and extractvalue to use constant indices instead of<br>Value* indices. And begin updating LangRef.html.</div></blockquote><div><br></div><div>You guys are making awesome progress, some thoughts:</div><br><div><br></div><div>Are insertvalue/extractvalue valid as constant expressions? If so, langref should be updated and it would be good to see that they encode into the bc file correctly. If we can *guarantee* that they are always folded away by the constant folding, then I'm fine with saying that they are not allowed as constant exprs.</div><div><br></div><div>In the insertvalue section, you have this syntax:</div><div><br></div><div><pre><span class="Apple-style-span" style="white-space: normal;"><result> = insertvalue <aggregate type> <val>, <ty> <val>, <idx> ; yields <n x <ty>></span></pre><pre><span class="Apple-style-span" style="white-space: normal; ">but this example:</span></pre></div><div>%result = insertvalue {i32, float} %agg, 1, 0</div><div><pre><span class="Apple-style-span" style="white-space: normal; ">should the example be:</span></pre><pre><span class="Apple-style-span" style="white-space: normal;"><pre><span class="Apple-style-span" style="white-space: normal;">%result = insertvalue {i32, float} %agg, i32 1, 0</span></pre></span></pre></div><div><br></div><blockquote type="cite"><div>+++ llvm/trunk/include/llvm/Constants.h Fri May 30 19:58:22 2008<br>public:<br> // Static methods to construct a ConstantExpr of different kinds. Note that<br>@@ -656,6 +657,10 @@<br> /// @brief Return true if this is a compare constant expression<br> bool isCompare() const;<br><br>+ /// @brief Return true if this is an insertvalue or extractvalue expression,<br>+ /// and the getIndices() method may be used.<br>+ bool hasIndices() const;<br>+</div></blockquote><blockquote type="cite"><div>+ /// getIndices - Assert that this is an insertvalue or exactvalue<br>+ /// expression and return the list of indices.<br>+ const SmallVector<unsigned, 4> &getIndices() const;</div></blockquote><div><br></div><div>Is there ever a case where insert/extract value can't be constant folded?</div><br><blockquote type="cite"><div>==============================================================================<br>--- llvm/trunk/include/llvm/Instructions.h (original)<br>+++ llvm/trunk/include/llvm/Instructions.h Fri May 30 19:58:22 2008<br>@@ -22,6 +22,7 @@<br> #include "llvm/DerivedTypes.h"<br> #include "llvm/ParameterAttributes.h"<br> #include "llvm/BasicBlock.h"<br>+#include "llvm/ADT/SmallVector.h"<br><br> namespace llvm {<br><br>@@ -1518,9 +1519,11 @@<br> /// element value from an aggregate value.<br> ///<br> class ExtractValueInst : public Instruction {<br>+ SmallVector<unsigned, 4> Indices;</div></blockquote><div><br></div><div>The indices of an instruction are fixed when the instruction is created. Would it be reasonable to just make this be a new unsigned[]'d array instead of a smallvector?</div><div><br></div><blockquote type="cite"><div>@@ -1563,9 +1567,9 @@<br><br> if (NumIdx > 0)<br> // This requires that the iterator points to contiguous memory.<br>- return getIndexedType(Ptr, (Value *const *)&*IdxBegin, NumIdx);<br>+ return getIndexedType(Ptr, (const unsigned *)&*IdxBegin, NumIdx);</div></blockquote><div><br></div>Is this cast needed? It seems that it could paper over serious bugs.</div><div><br></div><div><blockquote type="cite"><div>@@ -1575,55 +1579,53 @@<br> /// Constructors - These two constructors are convenience methods because one<br> /// and two index extractvalue instructions are so common.<br>- ExtractValueInst(Value *Agg, Value *Idx, const std::string &Name = "",<br>+ ExtractValueInst(Value *Agg, unsigned Idx, const std::string &Name = "",<br> Instruction *InsertBefore = 0);<br>- ExtractValueInst(Value *Agg, Value *Idx,<br>+ ExtractValueInst(Value *Agg, unsigned Idx,<br> const std::string &Name, BasicBlock *InsertAtEnd);</div></blockquote><div><br></div>Since the ctors are private, who are they a convenience for? It seems that the ::Create methods are the things that should exist, but extra constructors aren't need.</div><div><br><blockquote type="cite"><div>public:<br>+ // allocate space for exactly two operands<br>+ void *operator new(size_t s) {<br>+ return User::operator new(s, 1);<br>+ }</div></blockquote><div><br></div>Plz update comment.</div><div><br><blockquote type="cite"><div>@@ -1733,9 +1729,12 @@<br> /// value into an aggregate value.<br> ///<br> class InsertValueInst : public Instruction {<br>+ SmallVector<unsigned, 4> Indices;</div></blockquote><div><br></div>The above all applies to InsertValueInst as well.</div><div><br><blockquote type="cite"><div>+++ llvm/trunk/lib/VMCore/Constants.cpp Fri May 30 19:58:22 2008<br>@@ -537,15 +537,23 @@<br> /// Constants.cpp, and is used behind the scenes to implement<br> /// extractvalue constant exprs.<br> class VISIBILITY_HIDDEN ExtractValueConstantExpr : public ConstantExpr {</div></blockquote><div><br></div><div>Can't Extract/InsertValue always be constant folded? If so, there is no reason to be able to represent them.</div><div><br></div><blockquote type="cite"><div>@@ -718,6 +703,21 @@<br> return getOpcode() == Instruction::ICmp || getOpcode() == Instruction::FCmp;<br> }<br><br>+bool ConstantExpr::hasIndices() const {<br>+ return getOpcode() == Instruction::ExtractValue ||<br>+ getOpcode() == Instruction::InsertValue;<br>+}<br>+<br>+const SmallVector<unsigned, 4> &ConstantExpr::getIndices() const {<br>+ if (const ExtractValueConstantExpr *EVCE =<br>+ dyn_cast<ExtractValueConstantExpr>(this))<br>+ return EVCE->Indices;<br>+ if (const InsertValueConstantExpr *IVCE =<br>+ dyn_cast<InsertValueConstantExpr>(this))<br>+ return IVCE->Indices;<br>+ assert(0 && "ConstantExpr does not have indices!");<br>+}</div></blockquote><div><br></div><div>Eliminating them also allows these methods to go away. If you choose to keep them, please change getindices to be:</div><div><br></div><div>..</div><div><blockquote type="cite">+ return <span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">cast<InsertValueConstantExpr>(this)</span>->Indices;</blockquote></div><div><br></div><div>eliminating the assert(0)</div><blockquote type="cite"><div><br>+ assert(OpNo == 0 && "ExtractaValue has only one operand!");<br>+ const SmallVector<unsigned, 4> Indices = getIndices();</div></blockquote><div><br></div>No need to copy the vector here. <br><blockquote type="cite"><div><br>+ return<br>+ ConstantExpr::getExtractValue(Op, &Indices[0], Indices.size());<br> }<br>+ case Instruction::InsertValue: {<br>+ const SmallVector<unsigned, 4> Indices = getIndices();</div></blockquote><div><br></div>likewise.</div><div><br><blockquote type="cite"><div><br>+ return ConstantExpr::getInsertValue(Ops[0], Ops[1],<br>+ &Indices[0], Indices.size());<br>+ }<br>+ case Instruction::ExtractValue: {<br>+ const SmallVector<unsigned, 4> Indices = getIndices();</div></blockquote><div><br></div>likewise.</div><div><br></div><div>-Chris</div></body></html>