Rework of Vector/Scalar Classification

Project Infrastructure LLVM Dev List Archives
1

Here's a reworked patch to mark instructions and operands as vector or scalar.
It uses TableGen to infer the flags from types, allowing the user to override
with a "let isVector = 0" clause.

I decided to forego classifying MachineMemOperands for now in the interests of
getting this piece in. I still think we should add type information to
MachineMemOperands. Why throw away perfectly good information we have at
the time they're created?

Obviously I will submit the patch in pieces but I wanted to show what the
overall result will be.

Please review. Thanks!

                               -Dave

vector.patch (11.1 KB)

2

Ping?

                                 -Dave

3

Ping?

                                 -Dave

4

Your diff isn't clean and won't apply to mainline, you have some previously committed changes, like the extraneous #include of MachineMemOperand.h.

More significantly, as I mentioned before, I don't think this is a great way to go. For MachineOperands on X86, you either have a register operand (which is obvious whether it is vector or not) or you have a collection of addressing mode stuff, which is decomposed and "not an operand".

What is the expected use case for "vector" operands that are not registers? What do you plan to use this information for?

-Chris

5

Your diff isn't clean and won't apply to mainline, you have some
previously committed changes, like the extraneous #include of
MachineMemOperand.h.

Yes, I know. I was simply asking for a review. I can regenerate it
if you wish.

More significantly, as I mentioned before, I don't think this is a
great way to go. For MachineOperands on X86, you either have a
register operand (which is obvious whether it is vector or not) or you
have a collection of addressing mode stuff, which is decomposed and
"not an operand".

I don't see how a register MachineOperand is obviously a vector unless
there's type information somewhere that I'm missing.

That decomposed addressing stuff is interesting, because if one of
your addressing compoenent is a vector, you have a gather-scatter
situation.

What is the expected use case for "vector" operands that are not
registers? What do you plan to use this information for?

Well, as I explained earlier, I wanted to add type information to
MachinbeMemOperands so I could comment spills as either Vector or
Scalar. That's less important now, so that's why I decided to
drop that for the time being. I still think type information in
the MachineMemOperand is a good idea because it preserves useful
information longer. But I'll come back to that if/when I have
a stronger need.

As for this patch, which marks instructions and operands as vector,
I don't have an immediate use other than it originally drove the
vector/scalar spill marking before Evan (quite rightly) asked for
a more general approach to deciding if an instruction is a vector
operation. At that point I decided that the MachineMemOperand needed
to change to mark spills.

In the future, I could imagine this being useful for doing peeps
and other things where we might like to know the cost of an instruction,
analyze the % of a function vectorized, etc.

For now, I just don't want to lose this work. Even just applying this
and immediately reverting it is better than nothing because it will
at least be preserved in the repository history.

                            -Dave

6

Your diff isn't clean and won't apply to mainline, you have some
previously committed changes, like the extraneous #include of
MachineMemOperand.h.

Yes, I know. I was simply asking for a review. I can regenerate it
if you wish.

More significantly, as I mentioned before, I don't think this is a
great way to go. For MachineOperands on X86, you either have a
register operand (which is obvious whether it is vector or not) or you
have a collection of addressing mode stuff, which is decomposed and
"not an operand".

I don't see how a register MachineOperand is obviously a vector unless
there's type information somewhere that I'm missing.

It has a register which has a register class. It's true that LLVM
currently uses one register class for both scalar and vector uses
of XMM registers on x86, but you could argue that that's a problem
which should be fixed for other reasons anyway -- it may allow
the use of subreg operators to model referencing the first element
of a vector, which would allow the coalescer to coalesce them.

That decomposed addressing stuff is interesting, because if one of
your addressing compoenent is a vector, you have a gather-scatter
situation.

What is the expected use case for "vector" operands that are not
registers? What do you plan to use this information for?

Well, as I explained earlier, I wanted to add type information to
MachinbeMemOperands so I could comment spills as either Vector or
Scalar. That's less important now, so that's why I decided to
drop that for the time being. I still think type information in
the MachineMemOperand is a good idea because it preserves useful
information longer. But I'll come back to that if/when I have
a stronger need.

As for this patch, which marks instructions and operands as vector,
I don't have an immediate use other than it originally drove the
vector/scalar spill marking before Evan (quite rightly) asked for
a more general approach to deciding if an instruction is a vector
operation. At that point I decided that the MachineMemOperand needed
to change to mark spills.

In the future, I could imagine this being useful for doing peeps
and other things where we might like to know the cost of an instruction,
analyze the % of a function vectorized, etc.

The cost of an instruction depends on the opcode; Vector vs Scalar
isn't meaningful there. % of a function vectorized is a rough
heuristic at best, and we can't afford to carry around every bit of
information that someone might theoretically use for a rough heuristic
some day.

For now, I just don't want to lose this work. Even just applying this
and immediately reverting it is better than nothing because it will
at least be preserved in the repository history.

Please don't abuse the repository like this.

Dan

7

> I don't see how a register MachineOperand is obviously a vector unless
> there's type information somewhere that I'm missing.

It has a register which has a register class. It's true that LLVM
currently uses one register class for both scalar and vector uses
of XMM registers on x86, but you could argue that that's a problem
which should be fixed for other reasons anyway -- it may allow
the use of subreg operators to model referencing the first element
of a vector, which would allow the coalescer to coalesce them.

But until that happens, there is no alternative to the patch I submitted.

> For now, I just don't want to lose this work. Even just applying this
> and immediately reverting it is better than nothing because it will
> at least be preserved in the repository history.

Please don't abuse the repository like this.

So the answer is...no?

It's a bit frustrating to be told to rework a patch to operate in a
different (and better) fashion only to have it rejected.

                                -Dave

8

I don't see how a register MachineOperand is obviously a vector unless
there's type information somewhere that I'm missing.

It has a register which has a register class. It's true that LLVM
currently uses one register class for both scalar and vector uses
of XMM registers on x86, but you could argue that that's a problem
which should be fixed for other reasons anyway -- it may allow
the use of subreg operators to model referencing the first element
of a vector, which would allow the coalescer to coalesce them.

But until that happens, there is no alternative to the patch I submitted.

True, but there isn't anything using this information
(specifically, isVector flags on operands) anyway, unless
there's something we don't know about.

For now, I just don't want to lose this work. Even just applying this
and immediately reverting it is better than nothing because it will
at least be preserved in the repository history.

Please don't abuse the repository like this.

So the answer is...no?

My comment here was in response to your comment here.

It's a bit frustrating to be told to rework a patch to operate in a
different (and better) fashion only to have it rejected.

I've not followed closely enough to know who told you to do it
this way; if they think the new patch is good then that's fine
with me. It looks like it probably won't break anything.

It's still very unclear what's motivating this patch, except
that you seem to need it for your own internal reasons, and it's
inconvenient to maintain it in your own tree. There are
precedents for this kind of thing though.

Dan

9

> But until that happens, there is no alternative to the patch I submitted.

True, but there isn't anything using this information
(specifically, isVector flags on operands) anyway, unless
there's something we don't know about.

No, nothing currently.

> It's a bit frustrating to be told to rework a patch to operate in a
> different (and better) fashion only to have it rejected.

I've not followed closely enough to know who told you to do it
this way; if they think the new patch is good then that's fine
with me. It looks like it probably won't break anything.

Evan asked me to rework it. I'm glad he did, for I learned a few new TableGen
tricks. So the exercise wasn't a complete waste. I've decided I'm going to
file the patch away in a little "to-do" folder and when I need it I can pull
it back out. Until then, I don't see a reason to push it upstream if no one
else finds it especially useful at the moment.

It's more important I get other stuff merged anyway, so I don't think we need
to do anything more with this right now.

                              -Dave