This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Tidy up and organise better ARM.td. NFC.
ClosedPublic

Authored by javed.absar on Jul 11 2017, 4:56 AM.

Details

Summary

This patch tidies up and organises properly ARM.td so that it is easier to understand and extend in the future.

Diff Detail

Event Timeline

javed.absar created this revision.Jul 11 2017, 4:56 AM
fhahn edited edge metadata.Jul 11 2017, 11:14 AM

Looks like a NFC to me. I left some inline comments, mostly potential indentation issues and some comments I think are worth keeping, unless I missed something and they are invalid now.

lib/Target/ARM/ARM.td
88

Indent is off

118

Indent is now off, as in the line below

129–130

Indent is off

193–195

Indent is off now

257–260

Indent is off

341

Indent is off now I think

615

This is still true (it still uses CortexA8Model if I am not mistaken) and the fixme is still valid I think? Shouldn't we keep this fimex?

659

This is still true (it still uses CortexA9Model if I am not mistaken) and the fixme is still valid I think? Shouldn't we keep this fimex?

669

This is still true (it still uses CortexA9Model if I am not mistaken) and the fixme is still valid I think? Shouldn't we keep this fimex?

681

This is still true (it still uses CortexA9Model if I am not mistaken) and the fixme is still valid I think? Shouldn't we keep this fimex?

723

This is still true (it still uses CortexA8Model if I am not mistaken) and the fixme is still valid I think?

728

This is still true (it still uses CortexA8Model if I am not mistaken) and the fixme is still valid I think?

737

This is still true (it still uses CortexA8Model if I am not mistaken) and the fixme is still valid I think?

747

This is still true (it still uses CortexA8Model if I am not mistaken) and the fixme is still valid I think?

838

This comment is still true, no?

915

This comment is still true, no?

944

Do we need this empty comment line?

952

Do we need this empty comment line?

963

Do we need this comment line?

Thanks Florian. I will fix the indents where I missed out.

Regarding comments 'FIXME: XYZ has currently the same ProcessorModel as ABC', I removed the comments as they are unnecessary clutter repeated over and over in the file. The model used is explicit in next line e.g. "cyclone", swiftmodel .. ", and from the names it is clear that it is using a different model temporarily.

fhahn added a comment.Jul 12 2017, 4:56 AM

I don't mind either way about the comments, I think they might be valuable, as the FIXME makes the problem explicit (and grep-able), but it's very unlikely that anybody not familiar with the problem will come along and pick it up because of the FIXMEs. Just go with what @rovka or @rengolin prefer :)

javed.absar marked 8 inline comments as done.

Hi Florian:

I have fixed the changes you mentioned. For the mode 'fixme' I put in a common generic FIXME as reminder.
Thanks

Ping! Renato, Diana, Florian ?

fhahn accepted this revision.Jul 12 2017, 2:55 PM

I've spotted a few more cases where the indentation looks slightly off.

Besides those nits, LGTM. Please fix them before committing and hold off committing the change until tomorrow evening, so Renato and Diana have a little bit more time to raise concerns.

lib/Target/ARM/ARM.td
60

Indent off?

92

Indent off

180

indent off?

236

indent off?

265

indent off?

276

indent off?

280

Indent off?

361

indent off?

387

alignment off by 1 space?

402

alignment looks off by 2 spaces (same for the line below)

760

multiple spaces after and

This revision is now accepted and ready to land.Jul 12 2017, 2:55 PM

Thanks Florian.
Sure, will make the changes and wait till tomorrow before committing, in case Renato or Diana have further comments.

rovka accepted this revision.Jul 13 2017, 1:35 AM

I don't see anything wrong with this.
I'm not a huge fan of splitting string literals, since it makes it harder to grep for things, but I see there was already some precedent for that in the file and I assume this is what clang-format does too, so it's fine.

lib/Target/ARM/ARM.td
23

Nitpick: maybe move this down to line 469-ish, where the architectures are defined? At a superficial glance it doesn't seem to be needed before that.

fhahn added a comment.Jul 13 2017, 2:14 AM

In AArch64.td strings literals are either split up or the following indentation pattern is used

def FeatureSlowPaired128 : SubtargetFeature<"slow-paired-128",
    "Paired128IsSlow", "true", "Paired 128 bit loads and stores are slow">;

def FeatureAlternateSExtLoadCVTF32Pattern : SubtargetFeature<
    "alternate-sextload-cvt-f32-pattern", "UseAlternateSExtLoadCVTF32Pattern",
    "true", "Use alternative pattern for sextload convert to f32">;
lib/Target/ARM/ARM.td
248

indent off

255

indent off

280

indent off

This revision was automatically updated to reflect the committed changes.