This patch tidies up and organises properly ARM.td so that it is easier to understand and extend in the future.
Details
Diff Detail
Event Timeline
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 | |
119 | Indent is now off, as in the line below | |
130–131 | Indent is off | |
195–197 | Indent is off now | |
259–262 | Indent is off | |
345 | 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? | |
950 | Do we need this empty comment line? | |
958 | Do we need this empty comment line? | |
969 | 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.
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 :)
Hi Florian:
I have fixed the changes you mentioned. For the mode 'fixme' I put in a common generic FIXME as reminder.
Thanks
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 | |
181–182 | indent off? | |
237–238 | indent off? | |
266–268 | indent off? | |
278–280 | indent off? | |
284 | Indent off? | |
365–366 | indent off? | |
392–398 | alignment off by 1 space? | |
408 | alignment looks off by 2 spaces (same for the line below) | |
766 | multiple spaces after and |
Thanks Florian.
Sure, will make the changes and wait till tomorrow before committing, in case Renato or Diana have further comments.
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. |
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 | ||
---|---|---|
250 | indent off | |
257 | indent off | |
284 | indent off |
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.