Jakob Stoklund Olesen <jolesen at apple.com> writes:> I think the for loops have merit, but not the way you want to use them. > > Some target descriptions have many sequential definitions, for example PowerPC/PPCRegisterInfo.td: > > // Vector registers > def V0 : VR< 0, "v0">, DwarfRegNum<[77, 77]>; > def V1 : VR< 1, "v1">, DwarfRegNum<[78, 78]>; > ... > def V30 : VR<30, "v30">, DwarfRegNum<[107, 107]>; > def V31 : VR<31, "v31">, DwarfRegNum<[108, 108]>; > > Such 'embarrassingly sequential' definitions have no value, and I > would welcome a for-loop construct that can create them. I think this > was the primary motivation behind Che-Liang's proposal.Ok. We agree here.> I do NOT want to factor out redundancy from instruction definitions like this: > > multiclass PTX_FLOAT_3OP<string opcstr> { > def rr32 : InstPTX<(outs RegF32:$d), > (ins RndMode:$r, RegF32:$a, RegF32:$b), > !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; > def ri32 : InstPTX<(outs RegF32:$d), > (ins RndMode:$r, RegF32:$a, f32imm:$b), > !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; > def rr64 : InstPTX<(outs RegF64:$d), > (ins RndMode:$r, RegF64:$a, RegF64:$b), > !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; > def ri64 : InstPTX<(outs RegF64:$d), > (ins RndMode:$r, RegF64:$a, f64imm:$b), > !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; > } > > As repeated many times on this thread, the most common operation that > a .td file must support is looking up an instruction and figuring out > what its properties are and where they came from.Ok. What properties are most important to look up? I have found it really easy to just run "tblgen X86.td" and look at the output. That's really the canonical definition, right? Yes, I know it's an extra step but if one really can't puzzle out where an instruction came from, it's an easy one. To me, this: !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; is dangerously redundant. It's the patterns that are hardest to write so I'd rather get them right once or a few times rather than get them wrong in lots of places and have to go hunt and fix them. This could be mitigated somewhat by doing something like this: class binary_pattern<string opcstr, string type> { string pattern = !strconcat(opcstr, "$r."#type#"\t$d, $a, $b"); } multiclass PTX_FLOAT_3OP<string opcstr> { def rr32 : InstPTX<(outs RegF32:$d), (ins RndMode:$r, RegF32:$a, RegF32:$b), binary_pattern<opcstrm "f32">.pattern, []>; def ri32 : InstPTX<(outs RegF32:$d), (ins RndMode:$r, RegF32:$a, f32imm:$b), binary_pattern<opcstrm "f32">.pattern, []>; def rr64 : InstPTX<(outs RegF64:$d), (ins RndMode:$r, RegF64:$a, RegF64:$b), binary_pattern<opcstrm "f64">.pattern, []>; def ri64 : InstPTX<(outs RegF64:$d), (ins RndMode:$r, RegF64:$a, f64imm:$b), binary_pattern<opcstrm "f64">.pattern, []>; } Would something like that be acceptable? This is even more important for many of the Pat<> patterns which tend to be quite complex. I have found several cases where Pat<> patterns existed for some types of, say, add operation but not others, for no apparent reason other than someone added one and forgot to add the others. Pat<> patterns is the whole reason multidefs got developed in the first place. I wanted to define Pat<> patterns right in the same place the "ordinary" patterns got defined. There's much less chance of missing something or writing inconsistent patterns that way. For x86, I also find the encoding details at the top level very distracting. I was confused for months about what they meant and I didn't care. I just wanted to generate assembly code. SSE/AVX has a very regular encoding and I think it would be valuable to express that a bit more concisely. Some of that is done now but I think it could be better. I have a less well-defined idea of what that looks like as it's one of the things I don't particularly like about what I developed here. -Dave
greened at obbligato.org (David A. Greene) writes:> To me, this: > > !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; > !strconcat(opcstr, "$r.f32\t$d, $a, $b"), []>; > !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; > !strconcat(opcstr, "$r.f64\t$d, $a, $b"), []>; > > is dangerously redundant. It's the patterns that are hardest to write > so I'd rather get them right once or a few times rather than get them > wrong in lots of places and have to go hunt and fix them.Of course, I meant to include the pattern dags as well as the asm specifications here. The same thing can be done with patterns as I showed with the asm specs. -Dave
On Oct 7, 2011, at 2:23 PM, David A. Greene wrote:>> As repeated many times on this thread, the most common operation that >> a .td file must support is looking up an instruction and figuring out >> what its properties are and where they came from. > > Ok. What properties are most important to look up? I have found it > really easy to just run "tblgen X86.td" and look at the output. That's > really the canonical definition, right?In other words, .td files are write-only. This is not acceptable. It also doesn't answer '...and where they came from'.> This could be mitigated somewhat by doing something like this: > > class binary_pattern<string opcstr, string type> { > string pattern = !strconcat(opcstr, "$r."#type#"\t$d, $a, $b"); > } > > multiclass PTX_FLOAT_3OP<string opcstr> { > def rr32 : InstPTX<(outs RegF32:$d), > (ins RndMode:$r, RegF32:$a, RegF32:$b), > binary_pattern<opcstrm "f32">.pattern, []>; > def ri32 : InstPTX<(outs RegF32:$d), > (ins RndMode:$r, RegF32:$a, f32imm:$b), > binary_pattern<opcstrm "f32">.pattern, []>; > def rr64 : InstPTX<(outs RegF64:$d), > (ins RndMode:$r, RegF64:$a, RegF64:$b), > binary_pattern<opcstrm "f64">.pattern, []>; > def ri64 : InstPTX<(outs RegF64:$d), > (ins RndMode:$r, RegF64:$a, f64imm:$b), > binary_pattern<opcstrm "f64">.pattern, []>; > } > > Would something like that be acceptable?You are adding complexity for no benefit. Now I have to look up the definition of InstPTX AND binary_pattern? This is not helping. Please focus your efforts on making .td files easy to use as an instruction reference. Compressing them with clever abstractions is contrary to that. /jakob
On Oct 7, 2011, at 2:23 PM, David A. Greene wrote:> I have found several cases where Pat<> patterns > existed for some types of, say, add operation but not others, for no > apparent reason other than someone added one and forgot to add the > others.How about a tool for checking completeness and orthogonality of pattern sets? A pattern coverage mode for isel would also be interesting. /jakob -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20111007/2d6e572b/attachment.html>
Hi Jakob and David,
The for-loop inside multiclass definition does not have to add extra
abstraction layer. As in the pseudo codes that David wrote earlier
(see below), it only condenses the repetitive 'def' statements inside
the multiclass definition into a more compact and less copy-paste
style form, instead of encapsulating them somewhere outside the
multiclass definition. I believe this would be more readable and
easier to maintain than the repetitive copy-n-paste 'def' statements
as in the PTXInstInfo.td.
I understand if you don't want an extra layer of abstraction (which
adds extra looking-ups to someone reading td files), but I think we
can have for-loop inside a multiclass without abstractions.
--------------------
multiclass sse_binop<opcode> {
for type = [f32, f64, v4f32, v2f64]
regclass = [FP32, FP64, VR128, VR128]
suffix = [ss, sd, ps, pd] {
def !toupper(suffix)#rr : Instr<
[(set (type regclass:$dst), (type (opcode (type regclass:$src1),
(type regclass:$src2))))]>;
def !toupper(suffix)#rm : Instr<
[(set (type regclass:$dst), (type (opcode (type regclass:$src1),
(type addr:$src2))))]>;
}
}
--------------------
Regards,
Che-Liang
On Sat, Oct 8, 2011 at 7:24 AM, Jakob Stoklund Olesen <jolesen at
apple.com> wrote:>
> On Oct 7, 2011, at 2:23 PM, David A. Greene wrote:
>
>>> As repeated many times on this thread, the most common operation
that
>>> a .td file must support is looking up an instruction and figuring
out
>>> what its properties are and where they came from.
>>
>> Ok. What properties are most important to look up? I have found it
>> really easy to just run "tblgen X86.td" and look at the
output. That's
>> really the canonical definition, right?
>
> In other words, .td files are write-only. This is not acceptable.
>
> It also doesn't answer '...and where they came from'.
>
>> This could be mitigated somewhat by doing something like this:
>>
>> class binary_pattern<string opcstr, string type> {
>> string pattern = !strconcat(opcstr, "$r."#type#"\t$d,
$a, $b");
>> }
>>
>> multiclass PTX_FLOAT_3OP<string opcstr> {
>> def rr32 : InstPTX<(outs RegF32:$d),
>> (ins RndMode:$r, RegF32:$a, RegF32:$b),
>> binary_pattern<opcstrm
"f32">.pattern, []>;
>> def ri32 : InstPTX<(outs RegF32:$d),
>> (ins RndMode:$r, RegF32:$a, f32imm:$b),
>> binary_pattern<opcstrm
"f32">.pattern, []>;
>> def rr64 : InstPTX<(outs RegF64:$d),
>> (ins RndMode:$r, RegF64:$a, RegF64:$b),
>> binary_pattern<opcstrm
"f64">.pattern, []>;
>> def ri64 : InstPTX<(outs RegF64:$d),
>> (ins RndMode:$r, RegF64:$a, f64imm:$b),
>> binary_pattern<opcstrm
"f64">.pattern, []>;
>> }
>>
>> Would something like that be acceptable?
>
> You are adding complexity for no benefit. Now I have to look up the
definition of InstPTX AND binary_pattern? This is not helping.
>
> Please focus your efforts on making .td files easy to use as an instruction
reference.
>
> Compressing them with clever abstractions is contrary to that.
>
> /jakob
>
>
Jakob Stoklund Olesen <jolesen at apple.com> writes:> On Oct 7, 2011, at 2:23 PM, David A. Greene wrote: > >>> As repeated many times on this thread, the most common operation that >>> a .td file must support is looking up an instruction and figuring out >>> what its properties are and where they came from. >> >> Ok. What properties are most important to look up? I have found it >> really easy to just run "tblgen X86.td" and look at the output. That's >> really the canonical definition, right? > > In other words, .td files are write-only. This is not acceptable.How is that so?> It also doesn't answer '...and where they came from'.What do you mean by that? They came from the .td file, right? I'm really not understanding what you need here. You still haven't answered the question. What properties are most important to make plain?>> This could be mitigated somewhat by doing something like this: >> >> class binary_pattern<string opcstr, string type> { >> string pattern = !strconcat(opcstr, "$r."#type#"\t$d, $a, $b"); >> } >> >> multiclass PTX_FLOAT_3OP<string opcstr> { >> def rr32 : InstPTX<(outs RegF32:$d), >> (ins RndMode:$r, RegF32:$a, RegF32:$b), >> binary_pattern<opcstrm "f32">.pattern, []>; >> def ri32 : InstPTX<(outs RegF32:$d), >> (ins RndMode:$r, RegF32:$a, f32imm:$b), >> binary_pattern<opcstrm "f32">.pattern, []>; >> def rr64 : InstPTX<(outs RegF64:$d), >> (ins RndMode:$r, RegF64:$a, RegF64:$b), >> binary_pattern<opcstrm "f64">.pattern, []>; >> def ri64 : InstPTX<(outs RegF64:$d), >> (ins RndMode:$r, RegF64:$a, f64imm:$b), >> binary_pattern<opcstrm "f64">.pattern, []>; >> } >> >> Would something like that be acceptable? > > You are adding complexity for no benefit. Now I have to look up the > definition of InstPTX AND binary_pattern? This is not helping.It is NOT no benefit. The benefuit is writing patterns once and getting them to work everywhere. When we find a bug we fix it in once place. If that's not important to the LLVM community, that's fine. But don't claim there is no benefit.> Please focus your efforts on making .td files easy to use as an > instruction reference.I'm not sure what you mean. The Intel/AMD manuals are the reference. Anything else is going to be buggy. The .td files are an implementation of that reference.> Compressing them with clever abstractions is contrary to that.It doesn't have to be. It's clear that the LLVM leadership will not allow the sort of improvements I would like to see to ease maintenenace. That's ok, I'll live with it. But it's very unfortunate given the experience I have had working with the x86 SIMD implementation. -Dave
Jakob Stoklund Olesen <stoklund at 2pi.dk> writes:> On Oct 7, 2011, at 2:23 PM, David A. Greene wrote: > > I have found several cases where Pat<> patterns > existed for some types of, say, add operation but not others, for no > apparent reason other than someone added one and forgot to add the > others. > > How about a tool for checking completeness and orthogonality of pattern sets?Or how about just writing them once and have them apply to all relevant instructions?> A pattern coverage mode for isel would also be interesting.Interesting. Can you expand on your idea? Coverage of what? -Dave