[PATCH] D71620: [Attributor] AAValueConstantRange: Value range analysis using constant range

[Hideto Ueno via Phabricator via llvm-commits]

uenoku marked 3 inline comments as done.
uenoku added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4905
+    return ConstantRange::getFull(getBitWidth());
+  }
+
----------------
jdoerfert wrote:
> Nit: I'd move this into the state as a static member (`getWorstState`) taking a bit width. That makes it consistent with the IntegerStates.
Will do.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4980
+    return getKnown().intersectWith(SCEVR).intersectWith(LVIR);
+  }
+
----------------
jdoerfert wrote:
> Shouldn't we merge the SCEV and LVI information into the known state during intialization and then be done with it?
I didn't merge the SCEV and LVI information into initialization because I want to utilize the position where the value is used(=`CtxI`).  I mean, 
```
int abs(int u){
  if(u>=0){
    return u;
  }
  else 
    return -u;
}
```
In this case, the argument `u` has a different known range in each uses.  
Do you have a better way to handle these kinds of flow- and context-sensitive information?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5217
+        if (this == &LHSAA || this == &RHSAA)
+          return false;
+
----------------
jdoerfert wrote:
> I doubt this works to prevent PHI circles. If you have `%phi = phi [0, ...], [%bo1, ...]; ...; %bo0 = add %phi, 1; ...; %bo1 = add %bo0, %bo0` there is a circle but of size 3, which should not cause this check to trigger.
> 
> That said, do we really need it? If so, do we have a test to show that?
I agree that it seems we don't this check. I'll remove.


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

https://reviews.llvm.org/D71620