[PATCH] D16090: [SelectionDAG] Fold more offsets into GlobalAddresses

[Dan Gohman via llvm-commits]

sunfish marked 4 inline comments as done.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:397
@@ +396,3 @@
+    /// Match "(X shl/srl V1) & V2" where V2 may not be present.
+    bool MatchRotateHalf(SDValue Op, SDValue &Shift, SDValue &Mask);
+
----------------
arsenm wrote:
> Why does this need to be added here? I don't see a new use of this
It calls isConstantIntBuildVectorOrConstantInt, which is also being made a member function, so that we don't have to pass the TLI around. Or would it be better to just explicitly pass the TLI around so that these don't need to be member functions?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:782-783
@@ +781,4 @@
+  if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(N))
+    if (TLI.isOffsetFoldingLegal(GA) &&
+        GA->getOpcode() == ISD::GlobalAddress)
+      return GA;
----------------
arsenm wrote:
> The order of these checks should be swapped
Fixed.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3278-3281
@@ +3277,6 @@
+                                       const SDNode *N2) {
+  if (!TLI->isOffsetFoldingLegal(GA))
+    return SDValue();
+  if (GA->getOpcode() != ISD::GlobalAddress)
+    return SDValue();
+  const ConstantSDNode *Cst2 = dyn_cast<ConstantSDNode>(N2);
----------------
arsenm wrote:
> Ditto
Fixed.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3285
@@ +3284,3 @@
+    return SDValue();
+  uint64_t Offset = Cst2->getSExtValue();
+  switch (Opcode) {
----------------
arsenm wrote:
> This should be int64_t, since you use getSExtValue and negate it below 
We need to do the negate and the add as unsigned to avoid undefined behavior. But I can make the variable signed and just cast it at its uses. Fixed.


Repository:
  rL LLVM

http://reviews.llvm.org/D16090