[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-13 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added a comment.

> What is a "keep constructor"?

Good question, I'm not sure.  I think I meant to say "key constructors".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-13 Thread Jay Foad via Phabricator via cfe-commits 
foad added a comment.

What is a "keep constructor"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added a comment.

I'll take care of the DAG.getAllOnesConstant change, but i'd appreciate it if 
you could look at the NOT cases.  Running tests on the DAG.getAllOnesConstant 
patch now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added a comment.

Thank you for the detailed review Craig!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243
"Don't know how to expand this subtraction!");
-Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
-   DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-   VT));
+Tmp1 = DAG.getNode(
+ISD::XOR, dl, VT, Node->getOperand(1),

craig.topper wrote:
> This could use DAG.getNOT if you're willing to make that change.
I'd prefer to keep this one separate, it isn't related to APInt.



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12185
 else
-  OtherOp = DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Will do this in a followup



Comment at: llvm/lib/Target/Lanai/LanaiISelLowering.cpp:1403
 else
-  OtherOp =
-  DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl, VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

craig.topper wrote:
> I think we have DAG.getAllOnesConstant if you want to use it
Likewise



Comment at: llvm/unittests/ADT/APIntTest.cpp:29
 TEST(APIntTest, ShiftLeftByZero) {
-  APInt One = APInt::getNullValue(65) + 1;
+  APInt One = APInt::getZero(65) + 1;
   APInt Shl = One.shl(0);

craig.topper wrote:
> That's a strange way to write APInt(64, 1) unless there was some specific bug.
I agree, assume that this was testing some former bug.  Unit tests are designed 
to be weird ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper added a comment.

I think I read this patch too closely. I'll leave it up to you how much of this 
you want to do.




Comment at: llvm/include/llvm/IR/Constants.h:206
   /// Determine if the value is all ones.
   bool isMinusOne() const { return Val.isAllOnesValue(); }
 

isAllOnes()



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:171
   TrueIfSigned = true;
   return RHS.isAllOnesValue();
 case ICmpInst::ICMP_SGT: // True if LHS s> -1

isAllOnes since you're already in the area



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:174
   TrueIfSigned = false;
   return RHS.isAllOnesValue();
 case ICmpInst::ICMP_SGE: // True if LHS s>= 0

isAllOnes



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243
"Don't know how to expand this subtraction!");
-Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
-   DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-   VT));
+Tmp1 = DAG.getNode(
+ISD::XOR, dl, VT, Node->getOperand(1),

This could use DAG.getNOT if you're willing to make that change.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:965
+  DAG.getConstant(APInt::getAllOnes(BitTy.getSizeInBits()), DL, MaskTy);
   SDValue NotMask = DAG.getNode(ISD::XOR, DL, MaskTy, Mask, AllOnes);
 

I think this could also be DAG.getNOT but I'm less sure.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1212
+  DAG.getConstant(APInt::getAllOnes(VT.getScalarSizeInBits()), DL, VT);
   SDValue NotMask = DAG.getNode(ISD::XOR, DL, VT, Mask, AllOnes);
 

This could be DAG.getNOT



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4021
   // (X & (C l>>/<< Y)) ==/!= 0  -->  ((X <> Y) & C) ==/!= 0
   if (C1.isNullValue())
 if (SDValue CC = optimizeSetCCByHoistingAndByConstFromLogicalShift(

isZero()



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4030
   // all bits set:   (X | (Y<<32)) == -1 --> (X & Y) == -1
   bool CmpZero = N1C->getAPIntValue().isNullValue();
+  bool CmpNegOne = N1C->getAPIntValue().isAllOnes();

isNullValue() -> isZero(). I was going to say you could use N1C->isZero() but I 
think it would have to be N1C->isNullValue() because that is the ConstantSDNode 
interface.



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12185
 else
-  OtherOp = DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

I think we have DAG.getAllOnesConstant if you want to use it



Comment at: llvm/lib/Target/Lanai/LanaiISelLowering.cpp:1403
 else
-  OtherOp =
-  DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl, VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

I think we have DAG.getAllOnesConstant if you want to use it



Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1983
   Carry = DAG.getNode(M68kISD::ADD, DL, DAG.getVTList(CarryVT, MVT::i32), 
Carry,
   DAG.getConstant(NegOne, DL, CarryVT));
 

This is also getAllOnesConstant



Comment at: llvm/unittests/ADT/APIntTest.cpp:29
 TEST(APIntTest, ShiftLeftByZero) {
-  APInt One = APInt::getNullValue(65) + 1;
+  APInt One = APInt::getZero(65) + 1;
   APInt Shl = One.shl(0);

That's a strange way to write APInt(64, 1) unless there was some specific bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via cfe-commits 
lattner marked an inline comment as done.
lattner added inline comments.



Comment at: llvm/include/llvm/ADT/APInt.h:384
   /// value for the APInt's bit width.
   bool isMaxValue() const { return isAllOnesValue(); }
 

craig.topper wrote:
> isAllOnes()?
Yep, good catch, fixed thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Craig Topper via Phabricator via cfe-commits 
craig.topper added inline comments.



Comment at: llvm/include/llvm/ADT/APInt.h:384
   /// value for the APInt's bit width.
   bool isMaxValue() const { return isAllOnesValue(); }
 

isAllOnes()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via cfe-commits 
lattner added a comment.

This patch has a lot of noise, the important part is APInt.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits