[PATCH] D90328: Eliminates dead store of an exisiting value

Thu Jan 21 02:03:00 PST 2021

xbolva00 added a comment.

In D90328#2499595 <https://reviews.llvm.org/D90328#2499595>, @fhahn wrote:

> In D90328#2499556 <https://reviews.llvm.org/D90328#2499556>, @fhahn wrote:
>
>> In D90328#2499373 <https://reviews.llvm.org/D90328#2499373>, @nikic wrote:
>>
>>> Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=a3904cc77f181cff7355357688edfc392a236f5d&to=b5ccbbb5b1d8fc76e2291a860e4cf0a4787b77d4&stat=instructions Seems mostly fine, only outlier is kimwitu++ in NewPM-O3 configuration (k.cc has 1.25% regression).
>>>
>>> Something I don't understand about the general approach here is why we start at the `Def` store and walk all uses to find a `Use` store to eliminate. Wouldn't it be more efficient to start at the `Use` store, and check the defining access? With the current approach, I think we'll only find the cases where `Def` is the defining access of `Use`, as everything else would be an indirect use. This approach should also be much easier to make more powerful (by using `getClobberingMemoryAccess()`) without running into compile-time issues from visiting indirect uses.
>>
>> I don't remember if there was any particular reason to choose this approach to start with. I can't think of anything right now. Let me see if there are any cases that we would miss if we flip things.
>
> Ah I think I remember now. I think I had an extension in mind that also looked at the uses that are loads to remove any redundant loads read the same location. Not sure how relevant this is in practice. I'll check.

Looking at llvm tracker + “remove noop loads” it looks fine, maybe lencod regressed in CT a bit more than expected (0,5%)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90328