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

Repository
rL LLVM

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 ↗(On Diff #119568)

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

811 ↗(On Diff #119568)

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

812 ↗(On Diff #119568)

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

1889 ↗(On Diff #119568)

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?

1905 ↗(On Diff #119568)

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.

1911 ↗(On Diff #119568)

space between matchRegisterNameAlias and ()?

2589 ↗(On Diff #119568)

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

2590 ↗(On Diff #119568)

stray new line?

2771 ↗(On Diff #119568)

unrelated change?

javed.absar added inline comments.Oct 19 2017, 9:45 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62 ↗(On Diff #119568)

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 ↗(On Diff #119568)

@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?

811 ↗(On Diff #119568)

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 ↗(On Diff #119568)

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.

1905 ↗(On Diff #119568)

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
813 ↗(On Diff #119661)

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

bool isVector() const { return Kind == k_Register && !Reg.isScalar; }
1898 ↗(On Diff #119661)

function naming consistency.

1912 ↗(On Diff #119661)

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

62 ↗(On Diff #119568)

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

2771 ↗(On Diff #119568)

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
813 ↗(On Diff #119661)

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.

62 ↗(On Diff #119568)

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)

rengolin added inline comments.Oct 24 2017, 10:11 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
813 ↗(On Diff #119661)

Makes sense

rengolin added inline comments.Oct 24 2017, 10:13 AM
lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
62 ↗(On Diff #120089)

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 ↗(On Diff #120243)

nit: unrelated whitespace change

1914 ↗(On Diff #120243)

nit: unrelated whitespace change.

2589 ↗(On Diff #119568)

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 ↗(On Diff #120243)

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

2589 ↗(On Diff #119568)

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
2627 ↗(On Diff #121001)

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.