Complex patterns in .td file - a question
Robert Khasanov
rob.khasanov at gmail.com
Sat Aug 16 05:37:07 PDT 2014
Hi Adam,
2014-08-15 9:06 GMT+04:00 Adam Nemet <anemet at apple.com>:
> 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?
>
>
I checked there is no necessity to make separate VTInfo parent class,
ignore this naming.
> Anyhow, I’ll prepare a patch with this and you can take it from there.
>
>
Great!
> 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/20140816/13f38320/attachment.html>
More information about the llvm-commits
mailing list