Complex patterns in .td file - a question
Adam Nemet
anemet at apple.com
Thu Aug 14 22:06:25 PDT 2014
Hi Robert,
On Aug 14, 2014, at 10:02 AM, Robert Khasanov <rob.khasanov at gmail.com> wrote:
> Hi Elena and Adam,
>
>
> 2014-08-13 22:20 GMT+04:00 Adam Nemet <anemet at apple.com>:
> 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?
>
>
> I agree that code such !cast<blah>(xxx#eltnr#yyy) becomes unreadable and welcome your idea to centralize common mapping.
>
> If we choose this way, it could be easy to extend to AVX512VL. We can introduce something like this (assume that VTInfo is parent class for X86VectorVTInfo):
> class AVX512VLVectorVTInfo<int EltNum, ValueType EltVT, string suffix = ""> {
> VTInfo VTInfo512 = X86VectorVTInfo<EltNum, EltVT, VR512, suffix>;
> VTInfo VTInfo256 = X86VectorVTInfo<!shr(EltNum,1), EltVT, VR256X, suffix>;
> VTInfo VTInfo128 = X86VectorVTInfo<!shr(EltNum,2), EltVT, VR128X, suffix>;
> }
>
> and pass this class avx512_blah_vl multiclasses through argument.
Excellent. That looks great to me. Why does VTInfo need to be different from X86VectorVTInfo?
Anyhow, I’ll prepare a patch with this and you can take it from there.
> 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 I understand. in this way number of multiclasses in intermediate level equals to number of VL classes,doesn't it?
I think I understand what you’re asking and the answer is yes. For each VL multiclass there is another multiclass which hides the mapping and is specific to that VL multiclass.
> 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.
>
> If we will use X86VectorVTInfo with this idea, I think it would be better to include local definitions in multiclasses like avx512_valign, avx512_binop_rm. There we can expand RC, VT and other arg and use them avx512_masking multiclass w/o casts.
OK. Let’s do this. I’ll post a patch tomorrow with X86VectorVTInfo and a sample application in AVX512_valign. I think that that would be an improvement over what we have today. Then I keep looking at tablegen to implement local definitions.
Thanks for your feedback!
Adam
>
>
>
> 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.
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140814/71944603/attachment.html>
More information about the llvm-commits
mailing list