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

[Quentin Colombet via llvm-commits]

> On Jul 7, 2016, at 1:31 PM, Tim Northover <t.p.northover at gmail.com> wrote:
> 
> 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"
> +#endif
> #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.

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 http://llvm.org/PR26576 <http://llvm.org/PR26576> but you already know about it :))

> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D22008
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160707/2addc0cd/attachment.html>