[PATCH] D22008: GlobalISel: implement low-level type suitable for MachineInstr selection

Tim Northover via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 7 13:31:08 PDT 2016

t.p.northover marked 9 inline comments as done.

Comment at: include/llvm/CodeGen/LowLevelType.h:83
@@ +82,3 @@
+  int getNumElements() const {
+    assert(isVector() && "cannot get number of elements on scalar/aggregate");
qcolombet wrote:
> qcolombet wrote:
> > Add a comment saying this should be call only for vector type.
> > /// \pre isVector() == true.
> unsigned instead of int.
I'll use a normal comment because "\pre" seems to be ignored by our documentation generator (and I prefer it).

Comment at: include/llvm/CodeGen/LowLevelType.h:136
@@ +135,3 @@
+  LLT doubleElements() const {
+    assert(isVector());
+    return LLT{Kind, NumElements * 2, ScalarSize};
eli.friedman wrote:
> Should this work on a scalar type?  (`s32` -> `<2 x s32>`).
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.

Comment at: include/llvm/CodeGen/MachineInstr.h:28
@@ -26,1 +27,3 @@
+#include "llvm/CodeGen/LowLevelType.h"
 #include "llvm/IR/DebugLoc.h"
qcolombet wrote:
> The ifdef is probably useless.
Why? It seems rather weird to #ifdef the use of LLT but not its definition.



More information about the llvm-commits mailing list