This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Normalize naming on keep constructors / predicate methods.
ClosedPublic

Authored by lattner on Sep 8 2021, 10:19 PM.

Details

Summary

This renames the primary methods for creating a zero value to getZero
instead of getNullValue and renames predicates like isAllOnesValue
to simply isAllOnes. This achieves two things:

  1. This starts standardizing predicates across the LLVM codebase, following (in this case) ConstantInt. The word "Value" doesn't convey anything of merit, and is missing in some of the other things.
  1. Calling an integer "null" doesn't make any sense. The original sin here is mine and I've regretted it for years. This moves us to calling it "zero" instead, which is correct!

APInt is widely used and I don't think anyone is keen to take massive source
breakage on anything so core, at least not all in one go. As such, this
doesn't actually delete any entrypoints, it "soft deprecates" them with a
comment.

Included in this patch are changes to a bunch of the codebase, but there are
more. We should normalize SelectionDAG and other APIs as well, which would
make the API change more mechanical.

Diff Detail

Event Timeline

lattner created this revision.Sep 8 2021, 10:19 PM
lattner requested review of this revision.Sep 8 2021, 10:19 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptSep 8 2021, 10:19 PM

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

craig.topper added inline comments.Sep 8 2021, 10:30 PM
llvm/include/llvm/ADT/APInt.h
384

isAllOnes()?

lattner marked an inline comment as done.Sep 8 2021, 10:45 PM
lattner added inline comments.
llvm/include/llvm/ADT/APInt.h
384

Yep, good catch, fixed thanks!

lattner updated this revision to Diff 371507.Sep 8 2021, 10:46 PM
lattner marked an inline comment as done.

Fix a comment typo, adopt isAllOnes() in APInt.h as suggested by Craig

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

llvm/include/llvm/IR/Constants.h
206

isAllOnes()

llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
171

isAllOnes since you're already in the area

174

isAllOnes

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3243

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

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
965

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

1212

This could be DAG.getNOT

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
4021

isZero()

4030

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.

llvm/lib/Target/ARM/ARMISelLowering.cpp
12185

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

llvm/lib/Target/Lanai/LanaiISelLowering.cpp
1403

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

llvm/lib/Target/M68k/M68kISelLowering.cpp
1983

This is also getAllOnesConstant

llvm/unittests/ADT/APIntTest.cpp
29

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

martong removed a subscriber: martong.Sep 9 2021, 2:52 AM
lattner updated this revision to Diff 371629.Sep 9 2021, 9:33 AM
lattner marked 2 inline comments as done.

Adopt a few suggestions Craig points out.

lattner added inline comments.Sep 9 2021, 9:35 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3243

I'd prefer to keep this one separate, it isn't related to APInt.

llvm/lib/Target/ARM/ARMISelLowering.cpp
12185

Will do this in a followup

llvm/lib/Target/Lanai/LanaiISelLowering.cpp
1403

Likewise

llvm/unittests/ADT/APIntTest.cpp
29

I agree, assume that this was testing some former bug. Unit tests are designed to be weird ;-)

This revision is now accepted and ready to land.Sep 9 2021, 9:45 AM
This revision was landed with ongoing or failed builds.Sep 9 2021, 9:50 AM
This revision was automatically updated to reflect the committed changes.

Thank you for the detailed review Craig!

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.

foad added a comment.Sep 13 2021, 1:41 AM

What is a "keep constructor"?

What is a "keep constructor"?

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