[PATCH] D58981: CodeGenPrepare: preserve inbounds attribute when sinking GEPs

Tim Northover via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 12 07:19:22 PDT 2019


t.p.northover marked an inline comment as done.
t.p.northover added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1975
+    // Conservatively reject 'inbounds' mismatches.
+    if (InBounds != other.InBounds)
+      return MultipleFields;
----------------
dmgreen wrote:
> t.p.northover wrote:
> > dmgreen wrote:
> > > If one of the two is inbounds, does that not make the other inbounds? In other words, why would a mismatch in the inbounds flag matter?
> > A violation would mean that one of the instructions produces `undef` and the other produces a known result, so I believe we could combine them by stripping off the `inbounds` attribute on the merged instruction.
> > 
> > But personally I doubt it's worth the effort. As far as I know, front-ends don't actually mix the two (all C and C++ arithmetic is inbounds or via inttoptr/ptrtoint).
> I thought (correct me if I'm wrong) that a violation would produce poison. And because we know the results are used in a load/store, that would be UB. So we can presume that doesn't happen, and having inbounds on one implies inbounds on the other, providing they are otherwise identical.
> 
> I did see one example of this making a difference, although it was rather complex, quite a small change (and more often went up, not down). You are probably correct about it not being worth the effort.
> I thought (correct me if I'm wrong) that a violation would produce poison

Sorry, yes.

> And because we know the results are used in a load/store, that would be UB. So we can presume that doesn't happen, and having inbounds on one implies inbounds on the other.

I think we (may) know it's statically used, but there could still be a dynamic check that makes the inbounds poison innocuous but would cause bad things if the wrapping GEP suddenly became inbounds.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58981





More information about the llvm-commits mailing list