Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[llvm] Remove uses of Type::getPointerTo() (NFC)

Authored by JOE1994 on Jul 13 2023, 1:59 PM.



Partial progress towards removing in-tree uses of getPointerTo(),
by employing the following options:

  • Drop the call entirely if the sole purpose of it is to support a no-op bitcast (remove the no-op bitcast as well).
  • Replace with PointerType::get()/PointerType::getUnqual()

This is a NFC cleanup effort.

Diff Detail

Event Timeline

JOE1994 created this revision.Jul 13 2023, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 1:59 PM
JOE1994 requested review of this revision.Jul 13 2023, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 1:59 PM
JOE1994 updated this revision to Diff 540180.Jul 13 2023, 2:00 PM
  • Apply clang-format
JOE1994 added inline comments.Jul 13 2023, 2:03 PM

Given that this line is guarded by if (!PtrTy->isOpaque(),
I wasn't sure whether using PointerType::getUnqual(LLVMContext&) here is acceptable.

For now, I chose to play safe and just used PointerType::getUnqual(Type *) here.

I think getting rid of typed pointers should be an actual cleanup, not just a textual replacement. I outlined a few possible simplifications, but I didn't analyze deeply.

728 ↗(On Diff #540180)

This is the same as PtrTy.

7852 ↗(On Diff #540180)

isOpaque() always returns true, this whole block can be removed.


Isn't this a no-op? And the one above?


clang-format does poorly when argument comments are involved.

590 ↗(On Diff #540180)

The condition is always true because the bitcast can never be matched.


Would that be correct?


This cast looks no-op.

38 ↗(On Diff #540180)
JOE1994 updated this revision to Diff 557166.Sep 21 2023, 3:05 AM

Apply feedback.

Currently based on old commit from last July.
Needs rebasing.

JOE1994 added inline comments.Sep 21 2023, 3:07 AM

Yes, CreatePointerCast on line 713 is a no-op, given that Tmp1 is a pointer with address space 0
and that the cast target type (on line 713) is also a pointer type with address space 0.

If Ptr is guaranteed to have address space 0, CreatePointerCast on line 710 would also be a no-op.

Given the comment on line 695 (added in f5d07a05bbd41f827ccfa1bed7bfdfbab2be85dc),
the original author likely meant to do bitcasts but just used pointer casts.

Removing both pointercasts doesn't cause any regressions in ninja check-llvm.


LIT test LLVM :: Transforms/PGOProfile/counter_promo_with_bias.ll fails with this change.

The suggested change should be updated to be Addr = Builder.Insert(AddrInst->clone());.

Though even after that,
in the suggested change Addr is IntToPtr (OrigBiastInst),
while in previous code Addr is IntToPtr (BiasInst->clone()) .


Thank you for noticing this.
Removing the pointercast doesn't cause any regressions in ninja check-llvm.

JOE1994 updated this revision to Diff 557168.Sep 21 2023, 3:48 AM

Rebased onto latest main

Would it be alright to merge this? After reading onto latest main, I think the associated changes are small and straightforward :)

This revision is now accepted and ready to land.Sep 22 2023, 2:15 PM
This revision was automatically updated to reflect the committed changes.