[PATCH] D106947: [DSE][NFC] Rename Later->Killing, Earlier->Killed

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 6 03:45:28 PDT 2021


fhahn added a comment.

In D106947#2972003 <https://reviews.llvm.org/D106947#2972003>, @ebrevnov wrote:

> In D106947#2953538 <https://reviews.llvm.org/D106947#2953538>, @asbirlea wrote:
>
>> Thank you for doing this cleanup!
>> I agree the new names are more descriptive, but my feedback is that the code becomes very hard to read. I for one have a hard time following where in the code it says `Killing` and where `Killed` after the refactoring.
>> May I suggest using `Killing` and `Dead` (or variations of `PotentiallyDead`/`MaybeDead` etc)? Happy to hear other suggestions of names that have different "root" not just different declension.
>> Disclaimer: I'm not a native speaker either, just giving some thoughts  :-).
>
> "Killing" and "Dead" looks OK for me. "Maybe" prefix already reserved for Optional values in the code. If there are no other suggestions in next few days I will go with this variant. Thanks!

Killing/Dead makes it easier to differentiate the two I think in general and looks good.

I think there are some places where the killing/dead terminology is a bit inaccurate for intermediate steps, e.g the first changed comment ` // 1. Get the next dominating clobbering MemoryDef (KilledAccess) by walking`. At this point, it is not yet known whether the access is dead/killed. It is just a subtle difference in wording, but I think it might be worth to try to encode it in the name. Using `MaybeDead/PotentiallyDead` as  suggested by @asbirlea  in such cases seem good options. IMO the `Maybe` prefix is easier to follow and more compact and personally I am not too worried about it also being used for `Optional`.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1356
 
   // Find a MemoryDef writing to \p DefLoc and dominating \p StartAccess, with
   // no read access between them or on any other path to a function exit block
----------------
comment here needs also updating


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106947



More information about the llvm-commits mailing list