[PATCH] D22008: GlobalISel: implement low-level type suitable for MachineInstr selection
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 7 13:48:26 PDT 2016
> 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>
More information about the llvm-commits
mailing list