<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 7, 2016, at 1:31 PM, Tim Northover <<a href="mailto:t.p.northover@gmail.com" class="">t.p.northover@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">t.p.northover marked 9 inline comments as done.<br class=""><br class="">================<br class="">Comment at: include/llvm/CodeGen/LowLevelType.h:83<br class="">@@ +82,3 @@<br class="">+<br class="">+ int getNumElements() const {<br class="">+ assert(isVector() && "cannot get number of elements on scalar/aggregate");<br class="">----------------<br class="">qcolombet wrote:<br class=""><blockquote type="cite" class="">qcolombet wrote:<br class=""><blockquote type="cite" class="">Add a comment saying this should be call only for vector type.<br class="">/// \pre isVector() == true.<br class=""></blockquote>unsigned instead of int.<br class=""></blockquote>I'll use a normal comment because "\pre" seems to be ignored by our documentation generator (and I prefer it).<br class=""><br class="">================<br class="">Comment at: include/llvm/CodeGen/LowLevelType.h:136<br class="">@@ +135,3 @@<br class="">+ LLT doubleElements() const {<br class="">+ assert(isVector());<br class="">+ return LLT{Kind, NumElements * 2, ScalarSize};<br class="">----------------<br class="">eli.friedman wrote:<br class=""><blockquote type="cite" class="">Should this work on a scalar type? (`s32` -> `<2 x s32>`).<br class=""></blockquote>For symmetry with halfElements, probably. It would make `MoreElements` also act as "Vectorize" in the same way `FewerElements` doubles as Scalarize which doesn't seem like a bad thing.<br class=""><br class="">================<br class="">Comment at: include/llvm/CodeGen/MachineInstr.h:28<br class="">@@ -26,1 +27,3 @@<br class="">+#include "llvm/CodeGen/LowLevelType.h"<br class="">+#endif<br class=""> #include "llvm/IR/DebugLoc.h"<br class="">----------------<br class="">qcolombet wrote:<br class=""><blockquote type="cite" class="">The ifdef is probably useless.<br class=""></blockquote>Why? It seems rather weird to #ifdef the use of LLT but not its definition.<br class=""></div></div></blockquote><div><br class=""></div><div>Sorry I should have expressed myself better. The ifdef is not needed for correctness and we want to limit our number of ifdefs, so I believe that one could be left out. That being said, I am fine with it for now and anyway cleaning the few ifdefs left is also in my todolist (in particular the type on the instruction, see <a href="http://llvm.org/PR26576" class="">http://llvm.org/PR26576</a> but you already know about it :))</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><br class="">Repository:<br class=""> rL LLVM<br class=""><br class=""><a href="http://reviews.llvm.org/D22008" class="">http://reviews.llvm.org/D22008</a><br class=""><br class=""><br class=""><br class=""></div></div></blockquote></div><br class=""></body></html>