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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 4 inline comments as done.
t.p.northover added a comment.

> Please add context

Very sorry about that. I used `git format-patch` and I don't have the -U9999 muscle memory there. Updated patch should have it.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1975
+    // Conservatively reject 'inbounds' mismatches.
+    if (InBounds != other.InBounds)
+      return MultipleFields;
----------------
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).


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2077
+  if (InBounds)
+    OS << "(inbounds)";
   if (BaseGV) {
----------------
dmgreen wrote:
> I would drop the brackets, but it's only debug output so up to you.
I can replace them by a space, which looks a bit more natural. We don't want it running into the following stuff.


================
Comment at: llvm/test/CodeGen/Thumb/addr-modes.ll:37
 ; Expected: GEP can be folded into LOAD.
-; CHECK: local addrmode: [2*%x]
+; CHECK: local addrmode: [(inbounds)2*%x]
 define i32 @load03(i32 %x) nounwind {
----------------
dmgreen wrote:
> This isn't really inbounds, is it? But we don't use that info here, (we don't create a gep), so it's alright? It's a little confusing, at least
I think you were right to be suspicious, it could theoretically be turned into a `GEP i16, null, %x` or something and the tag would be wrong there. I've fixed it in the updated patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58981