[PATCH] D92045: [DSE] Consider out-of-bound writes in isOverwrite.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/out-of-bounds-stores.ll:73
   ret i32 0
 }
----------------
nikic wrote:
> nikic wrote:
> > fhahn wrote:
> > > nikic wrote:
> > > > I'm having a hard time understanding why anything gets eliminated in this example, regardless of whether this is considered an overwrite or not. There is a codepath (for.body -> for.inc -> for.end) that goes from the first store to the load without passing through the store in for.body.1, so how can it even be relevant for DSE purposes?
> > > The problem here is that we start looking upwards with the location of the potential killing definition. So when going upwards we consider `store i32 10, i32* %arrayidx, align 4` to be completely overwritten by `store i32 20, i32* %arrayidx.1, align 4`, which is out-of-bounds. We then check for read clobbers starting at `store i32 10, i32* %arrayidx, align 4`, we use the location of the killing def, which does not alias the read.
> > > 
> > > It might be beneficial to use the location of the store to eliminate for the read check for the full overwrite case at least. But I think that would be better as follow up.
> > Okay, I think I get it now. The problem is that we're using the MemoryLocation from the store in `for.body.1` when looking for read clobbers of the store in `for.body`. And AA (correctly) reports it is NoAlias with the read in `for.end`. So we have a contract here that the isOverwrite logic in DSE and AA do not produce contradictory results.
> > It might be beneficial to use the location of the store to eliminate for the read check for the full overwrite case at least. But I think that would be better as follow up.
> 
> I think this is what we should be doing. IMHO this patch is only a workaround for the real issue, and we'd just have to revert it again once the proper location is used.
> 
> I tried a minimal patch (https://gist.github.com/nikic/83b3207c0581cbbcc7d95a5a20c23d93) but got a test-suite failure in clamscan with that, so would need further investigation.
> I think this is what we should be doing. IMHO this patch is only a workaround for the real issue, and we'd just have to revert it again once the proper location is used.
> 

Hm, I am not sure. The issue is that `isOverwrite` did not have to handle this case before I think.

> I tried a minimal patch (https://gist.github.com/nikic/83b3207c0581cbbcc7d95a5a20c23d93) but got a test-suite failure in clamscan with that, so would need further investigation.
> 

Yeah, I also had a quick look and it seems there are a few more adjustments needed. I won't have time to dig too deep in the next week or 2 unfortunately. I think it would be preferable to fix the issue with this patch and then follow-up later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92045