This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC)
ClosedPublic

Authored by sdesmalen on Oct 19 2017, 5:01 AM.

Details

Summary

Patch [2/5] in a series to add assembler/disassembler support for AArch64 SVE unpredicated ADD/SUB instructions.

This change is a non functional change that adds RegKind as an alternative to 'isVector' to prepare it for newer types (SVE data vectors and predicate vectors) that will be added in next patches (where the SVE data vector is added as part of this patch set)

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 19 2017, 5:01 AM
fhahn added a subscriber: fhahn.Oct 19 2017, 9:33 AM
fhahn added inline comments.
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

Would it make sense to use something more generic for the names here, like ScalableVector?

810

Personally I think having predicate functions like Reg.isScalar(), Reg.isNeonVector() would be slightly more compact, but I think either way is fine.

811

From looking at the functions around here, it seems like a space would be good here.

1889

What is the reason for moving this code out to a separate function? Is it to not change the interface of matchRegisterNameAlias, which still takes bool isVector as an argument?

1890–1891

I think keeping the boolean isVector argument has potential to confuse people. Any reason for not being explicit here and pass RegKind here? Because there are 2 vector register types now, isVector seems ambiguous.

1914

space between matchRegisterNameAlias and ()?

2592–2593

Any reason for not passing RegKind::NeonVector directly here? It seems like RegisterKind is not used anywhere else around here.

2593

stray new line?

2774

unrelated change?

javed.absar added inline comments.Oct 19 2017, 9:45 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

It seems to be it would be more maintainable to split it

  • RegKind { Scalar, Vector }; and then

enum class VectorKind { NeonVector, SVEVector};

This might simplify the checking logic further down.

sdesmalen updated this revision to Diff 119661.Oct 20 2017, 8:39 AM
sdesmalen marked 3 inline comments as done.

Refactored code to reflect some of the suggestions.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

@fhahn A subsequent patch will also add a SVE Predicate Vector.

@javed.absar I had a look at splitting it into RegKind { Scalar, Vector }; and subsequently into VectorKind {NeonVector, SVEVector}, but I don't easily see how this makes the checking logic any simpler. Did you have a specific check that you think would be simplified by this?

810

I think that is basically what we're implementing here, but hen for 'isReg()'. See for example 'isVectorReg()', which is the opposite of 'isScalar()'. I see now that isVectorReg() is only checking for a NeonVector, which seems inconsistent with its name. I'll see if I can fix that.

1889

We initially moved it into a separate function to reuse it for 'matchSVEVectorNameAlias()' (as added in patch 3/5)... But, looking at it again I see it makes more sense to add to the same function and use RegKind for each (which eliminates the purpose of moving this into a separate function). Please check if you're happy with the next iteration of this patch.

1890–1891

should be addressed by the next iteration of this patch (see previous comment)

rengolin edited edge metadata.Oct 23 2017, 2:26 PM

So, overall, this is a sensible change. We must be able to separate Neon vectors from SVE vectors. I'm not sure how the predicates will fare here and why they wouldn't be SVE vectors (surely, they're scalable, too).

I would prefer to keep isVector and add isScalable, and move the distinction between Neon and SVE to helper functions. But I'm not holding this opinion firmly, as I haven't seen the rest of the code.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

Why won't SVE predicate not be an "SVEVector"?

813

Is there a situation (in the future) that we might want?

bool isVector() const { return Kind == k_Register && !Reg.isScalar; }
1898

function naming consistency.

1913

not that it matters much but the return change here is pointless. :)

2774

just checked, this is not violating 80-columns. :)

sdesmalen updated this revision to Diff 120089.Oct 24 2017, 9:57 AM
sdesmalen marked 3 inline comments as done.
sdesmalen edited the summary of this revision. (Show Details)
sdesmalen added inline comments.
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

Fair point, it should probably distinguish Data and Predicate in the name. I'll change this.
(also, since this is not yet used until the next patch in this series, I will move this change to that patch)

813

At the moment I haven't found such a situation yet (even when considering other SVE assembler/disassembler patches) and I would rather not add code that isn't used anywhere.
So I suggest adding that only when needed.

rengolin added inline comments.Oct 24 2017, 10:11 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
813

Makes sense

rengolin added inline comments.Oct 24 2017, 10:13 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62

nit: space alignment.

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

sdesmalen updated this revision to Diff 120243.Oct 25 2017, 6:58 AM

Removed whitespace.

sdesmalen marked an inline comment as done.Oct 25 2017, 6:58 AM
fhahn added a comment.Oct 31 2017, 6:53 AM

A few minor nits, but this looks good to me now. @rengolin what do you think?

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
1909

nit: unrelated whitespace change

1914

nit: unrelated whitespace change.

2592–2593

Do we need the RegisterKind variable? Doesn't look like it is used somewhere else.

sdesmalen updated this revision to Diff 121001.Oct 31 2017, 9:48 AM
sdesmalen marked an inline comment as done.
  • Propagated 'RegKind::NeonVector' for variable RegisterKind with single use.
  • Removed newline.
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
1909

I don't feel strongly about it, but I think it improves readability.

2592–2593

I thought I had addressed it in my previous patch, but I must have missed it. Please see updated patch.

Small nit, but overall, looks good to me.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2628

nit: shouldn't this function be called tryParseNeonVectorRegister, then?

fhahn added a comment.Nov 2 2017, 3:35 AM

Thanks Sander, looks good to me too, with the nit @rengolin pointed out.

sdesmalen updated this revision to Diff 121481.Nov 3 2017, 8:13 AM
sdesmalen marked an inline comment as done.

Renamed tryParseVectorRegister into tryParseNeonVectorRegister.

This looks good to me, thanks!

Let's wait for a final round of comments.

fhahn added a comment.Nov 6 2017, 9:27 AM

Looks good to me now.

rengolin accepted this revision.Nov 6 2017, 9:27 AM
This revision is now accepted and ready to land.Nov 6 2017, 9:27 AM
This revision was automatically updated to reflect the committed changes.