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

[Youngsuk Kim via Phabricator via llvm-commits]

JOE1994 added inline comments.


================
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.
 
----------------
DanielMcIntosh-IBM wrote:
> 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.
Thank you for your comment. My understanding was that the term 'commutable' has only one definition ("allowing regular commuting to and from work"), so I was almost confident that this word was used here by mistake. If the term 'commutable' is already a frequently used term when discussing traits of operators/instructions, I'd be happy to get rid of this change.


================
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
----------------
DanielMcIntosh-IBM wrote:
> 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!")
I initially thought my change would make it more clear for the phrase to have the former meaning that you suggested.
Maybe it might not be the case.. I'll get rid of this change.


================
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:
 
----------------
DanielMcIntosh-IBM wrote:
> 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.
I will keep this unchanged. Thank you for your comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127701