[PATCH] D30416: [BitfieldShrinking] Shrink Bitfields load/store when the bitfields are legal to access independently

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Digging into the code next, but wanted to send some comments just on terminology and the documentation while I'm doing that.



================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:15
+/// A major motivation of the optimization:
+/// Consecutive bitfields are now wrapped as a group and represented as a
+/// large integer. To access a specific bitfield, wide load/store plus a
----------------
nit: s/now//


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:26
+///
+/// List some typical patterns and the way to transform them, in order to
+/// give an idea how the optimization works. More general patterns to recognize
----------------
This sentence doesn't really parse for me, it reads as a command rather a description and comments typically are descriptive.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:46
+/// * The algorithm scans the program and tries to recognize and transform the
+///   patterns above. It runs iteratively until no more change has happened.
+/// * To prevent the optimization from blocking load/store coalescing, it is
----------------
"no more change has happened" -> "no more changes have happened"


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:50
+///   stage, both the pattern matching and related safety check become more
+///   difficult because of previous optimizations introducing mergd load/store
+///   and more complex control flow. That is why MemorySSA is used here. It is
----------------
"mergd" -> "merged"

"load/store" needs to be "loads and stores" or "load/store instructions".


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:52
+///   and more complex control flow. That is why MemorySSA is used here. It is
+///   scalable and precise for the safety check especially when we tries to
+///   insert a shrinked load in the block which is many blocks away from its
----------------
efriedma wrote:
> "try", not "tries".
I would reword this sentence to be a bit easier to read:

  It provides scalable and precise safety checks even when we try to insert
  a smaller access into a block which is many blocks away from the original
  access.


================
Comment at: lib/CodeGen/MemAccessShrinking.cpp:53
+///   scalable and precise for the safety check especially when we tries to
+///   insert a shrinked load in the block which is many blocks away from its
+///   original load.
----------------
A comment on the terminology throughout this patch: the adjective describing something which has been reduced in size in the past is "shrunken". That said, if this is awkward to use, I might use the adjective "smaller". But "shrinked" isn't a word in English.


Repository:
  rL LLVM

https://reviews.llvm.org/D30416