Complex patterns in .td file - a question
Adam Nemet
anemet at apple.com
Wed Aug 13 11:20:05 PDT 2014
Hi Elena and Robert,
On Aug 13, 2014, at 12:01 AM, Demikhovsky, Elena <elena.demikhovsky at intel.com> wrote:
> Hi Nadav, Craig, Adam,
>
> Robert works on SKX and I’m reviewing the code. I have a question about the form of patterns. Let’s assume that everything is correct here. Do you think that it is the suitable form? And shorter is better?
> Or on some stage it becomes unreadable?
Thanks very much for starting this discussion! I have been struggling to find the balance between generality and readability as well so it’s good to have this meta-discussion to make sure we’re on the same page.
I think that we should try to use tablegen to generate as much as possible. I believe that this is the only way to get the huge number of instruction-variants/intrinsics supported for AVX512 in a scalable/maintainable way. This is where I was going with the AVX512_masking classes and handling intrinsics via lowering to SDNodes.
At the same time we should try to separate the unreadable part (e.g. the !cast<blah>(xxx#eltnr#yyy) from the code that actually captures some important logic.
One idea I had is to introduce a class to capture all the different information we derive from the vector type. This is essentially used to hide and to centralize the nasty mappings. E.g.:
+// Group template arguments that can be derived from the vector type (EltNum x
+// EltVT). These are things like RegisterClass, the corresponding mask RC,
+// etc. The idea is to pass one of these as the template argument rather than
+// the individual arguments.
+class X86VectorVTInfo<int EltNum, ValueType EltVT, RegisterClass rc = VR512,
+ string suffix = ""> {
+ RegisterClass RC = rc;
+ RegisterClass KRC = !cast<RegisterClass>("VK"##EltNum##"WM");
+ RegisterClass MRC = !cast<RegisterClass>("GR"##EltNum);
+ string Suffix = suffix;
+ ValueType VT = !cast<ValueType>("v"##EltNum##EltVT);
+}
I am sure there is bunch more “field” to be added for BW/DQ/VL...
Then we can have an instance of this class for each vector type:
+def v16i32_info : X86VectorVTInfo<16, i32, VR512, "d">;
+def v8i64_info : X86VectorVTInfo<8, i64, VR512, "q”>;
And then we would pass these instead of the RC, VT, KRC, MRC, etc, to the classes/multiclasses. So instead of this:
-defm VALIGND : avx512_valign<"d", VR512, VK16WM, GR16, i512mem, v16i32, v16f32>,
- EVEX_V512, EVEX_CD8<32, CD8VF>;
-defm VALIGNQ : avx512_valign<"q", VR512, VK8WM, GR8, i512mem, v8i64, v8f64>,
- VEX_W, EVEX_V512, EVEX_CD8<64, CD8VF>;
We’ll have this (we can probably add the other args as well):
+defm VALIGND : avx512_valign1<v16i32_info, i512mem, v16f32>,
+ EVEX_V512, EVEX_CD8<32, CD8VF>;
+defm VALIGNQ : avx512_valign1<v8i64_info, i512mem, v8f64>,
+ VEX_W, EVEX_V512, EVEX_CD8<64, CD8VF>;
I didn’t get a chance to look at the VL stuff and see if this helps there or not. Robert, what do you think?
Besides this infrastructure (let me know if you think it’s a good idea) we can also use an intermediate level of multiclass to “expand” derived arguments on a case-by-case basis. This multiclass will not do anything other than translate the input args into derived args using the nasty !cast, stringconcats and what not. Then both avx512_int_broadcast_reg_pat and avx512_int_broadcast_reg_pat_vl should be a lot cleaner and hopefully the nasty code is contained in the intermediate class.
As an alternative/extension to the previous idea we could also allow “local” definitions in multiclasses (currently you can only have def, defm and let in a multiclass). Then we could write something like:
multiclass blah<string elsz, ….> {
RegisterClass MRC = !cast<RegisterClass>(“GR” # elsz); // or even better = X86VectorVTInfo<elty, elsz>.MRC
and then you could use MRC to formulate the logic without the concat/cast getting in the way of clarity:
> defm : avx512_int_broadcast_reg_pat<InstName##"Z256", X86VBroadcast,
> !cast<RegisterClass>("VK"##vsz256##"WM”),
> VR256X, !cast<RegisterClass>("GR"##elsz),
This would be the last line now:
VR256X, MRC,
> !cast<ValueType>("v"##vsz256##"i"##elsz),
> v8i32, !cast<ValueType>("i"##elsz), SubRI>;
I haven’t looked at how complicated this would be to add to tablegen.
Adam
>
> Thank you.
>
> multiclass avx512_int_broadcast_reg_pat<string InstName, SDNode OpNode,
> RegisterClass KRC, RegisterClass DestRC,
> RegisterClass SrcRC, ValueType vt,
> ValueType ivt, ValueType svt,
> string SubRI = ""> {
> def : Pat<(vt (OpNode (svt SrcRC:$src))),
> (!cast<Instruction>(InstName##"r")
> !if(!eq(SubRI, ""), (svt SrcRC:$src),
> (SUBREG_TO_REG (i32 0), SrcRC:$src, !cast<SubRegIndex>(SubRI))))>;
> let AddedComplexity = 30 in {
> def : Pat<(vt (vselect KRC:$mask, (OpNode (svt SrcRC:$src)),
> DestRC:$src0)),
> (!cast<Instruction>(InstName##"rk") DestRC:$src0, KRC:$mask,
> !if(!eq(SubRI, ""), (svt SrcRC:$src),
> (SUBREG_TO_REG (i32 0), SrcRC:$src, !cast<SubRegIndex>(SubRI))))>;
>
> def : Pat<(vt (vselect KRC:$mask, (OpNode (svt SrcRC:$src)),
> (vt (bitconvert (ivt immAllZerosV))))),
> (!cast<Instruction>(InstName##"rkz") KRC:$mask,
> !if(!eq(SubRI, ""), (svt SrcRC:$src),
> (SUBREG_TO_REG (i32 0), SrcRC:$src, !cast<SubRegIndex>(SubRI))))>;
> }
> }
>
> multiclass avx512_int_broadcast_reg_pat_vl<string InstName, string elsz,
> string vsz, string vsz256,
> string vsz128, Predicate prd,
> string SubRI = ""> {
> let Predicates = [prd] in
> defm : avx512_int_broadcast_reg_pat<InstName##"Z", X86VBroadcast,
> !cast<RegisterClass>("VK"##vsz##"WM"),
> VR512, !cast<RegisterClass>("GR"##elsz),
> !cast<ValueType>("v"##vsz##"i"##elsz),
> v16i32, !cast<ValueType>("i"##elsz), SubRI>;
> let Predicates = [prd, HasVLX] in {
> defm : avx512_int_broadcast_reg_pat<InstName##"Z256", X86VBroadcast,
> !cast<RegisterClass>("VK"##vsz256##"WM"),
> VR256X, !cast<RegisterClass>("GR"##elsz),
> !cast<ValueType>("v"##vsz256##"i"##elsz),
> v8i32, !cast<ValueType>("i"##elsz), SubRI>;
> defm : avx512_int_broadcast_reg_pat<InstName##"Z128", X86VBroadcast,
> !cast<RegisterClass>("VK"##vsz128##"WM"),
> VR128X, !cast<RegisterClass>("GR"##elsz),
> !cast<ValueType>("v"##vsz128##"i"##elsz),
> v4i32, !cast<ValueType>("i"##elsz), SubRI>;
> }
> }
>
> defm : avx512_int_broadcast_reg_pat_vl<"VPBROADCASTBr", "8", "64", "32", "16",
> HasBWI, "sub_8bit">;
> defm : avx512_int_broadcast_reg_pat_vl<"VPBROADCASTWr", "16", "32", "16", "8",
> HasBWI, "sub_16bit">;
> defm : avx512_int_broadcast_reg_pat_vl<"VPBROADCASTDr", "32", "16", "8", "4",
> HasAVX512>;
> defm : avx512_int_broadcast_reg_pat_vl<"VPBROADCASTQr", "64", "8", "4", "2",
> HasAVX512>, VEX_W;
>
> Elena
>
>
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140813/49f32e07/attachment.html>
More information about the llvm-commits
mailing list