[PATCH] D86862: [NFC] [DAGCombiner] Refactor bitcast folding within fabs/fneg

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21282-21283
+  EVT VT = N->getValueType(0);
+  bool isFNeg = N->getOpcode() == ISD::FNEG;
+  bool Free = isFNeg ? TLI.isFNegFree(VT) : TLI.isFAbsFree(VT);
+
----------------
Formatting nit: variables start with a capital letter, so "IsFNeg". But seeing the uses below, maybe this should be "IsFabs", so we don't have to use negative logic?

Also "Free" would be better as "IsFree".


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21287
+  // (fabs (bitconvert x)) -> (bitconvert (and x ~sign))
+  if (!Free && N0.getOpcode() == ISD::BITCAST && N0.hasOneUse()) {
+    SDValue Int = N0.getOperand(0);
----------------
Invert this logic and early exit, so:
  if (IsFree || ...) return SDValue();


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21293-21294
+      if (N0.getValueType().isVector()) {
+        // For a vector, get a mask such as 0x7f... per scalar element
+        // and splat it.
+        SignMask = APInt::getSignMask(N0.getScalarValueSizeInBits());
----------------
These comments would be clearer if they said something like:
  // Create a sign mask (0x80...) or its inverse (0x7f...). 


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

https://reviews.llvm.org/D86862