[PATCH] D127701: [docs] Minor fixes to CodeGenerator docs

[Daniel McIntosh via Phabricator via llvm-commits]

DanielMcIntosh-IBM requested changes to this revision.
DanielMcIntosh-IBM added a comment.
This revision now requires changes to proceed.

What was the motivation for this change?

Except for the change on line 619 to correct a mistake that was a little jarring to read, these changes seem to be mostly a matter of personal preference for how YOU would write this document (or rigid imposition of english grammar rules like when to use "less" or "fewer"), and I'm personally not a fan of trivial changes like this cluttering up the commit history and making it harder to run commands like git blame.

I know my comments below probably come off a bit harsh, and I'll say in advance that's not how I meant them. If it weren't for the change on line 422 I probably would have just resigned as a reviewer, since I don't have enough experience to know what the general attitude in the community is towards tiny inconsequential changes like this.



================
Comment at: llvm/docs/CodeGenerator.rst:291
 whether the instruction has certain target-independent properties (accesses
-memory, is commutable, etc), and holds any target-specific flags.
 
----------------
I don't think this actually improves anything, except maybe removing ambiguity with the other definition of commutable ("allowing regular commuting to and from work"). If anything I feel the existing wording reads better (whether the instruction ... accesses memory, whether the instruction ... is commutable reads better to me than whether the instruction ... is commutative). Unless I'm missing something, this is a matter of personal preference and not worthy of making a change for.


================
Comment at: llvm/docs/CodeGenerator.rst:422
 in a particular register.  This can occur due to limitations of the instruction
-set (e.g., the X86 can only do a 32-bit divide with the ``EAX``/``EDX``
 registers), or external factors like calling conventions.  In any case, the
----------------
This dramatically changes the meaning - The current form implies a limitation on the X86 divide operation ("The x86 architecture cannot use any registers except EAX and EDX for the divide operation"), while the edited form describes a feature ("Unlike other architectures the x86 can do a 32-bit divide using nothing but the EAX and EDX registers!")


================
Comment at: llvm/docs/CodeGenerator.rst:1108
 While it has many strengths, the system currently has some limitations,
-primarily because it is a work in progress and is not yet finished:
 
----------------
This does change the meaning slightly. "Is not yet finished" would imply there is a 'finished' or completed state that it hasn't yet achieved. Compare a city's infrastructure to an early access video-game. A city's infrastructure is a perpetual work in progress - always getting improved upon, repaired, expanded, etc. but there's no 'finish line'. In contrast, the early access video game has a goal that it's trying to get to. It's not that everything stops after the finish line is crossed, just that before then, it's missing major features and can't be considered a complete product. Development might not even slow down after the full release, but there shouldn't be any more loose ends.

I don't know which is more accurate here, since I don't know enough about the CodeGenerator to comment, but going based on the rest of your changes, it seems like you might be removing this because you felt it was superfluous, and I don't agree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127701/new/

https://reviews.llvm.org/D127701