[PATCH] D128670: [SimplifyCFG] teach simplifycfg not to introduce ptrtoint for NI pointers

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LG



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:483
+      DL.isNonIntegralPointerType(V->getType()))
     return CI;
 
----------------
I realize that this is pre-existing, but I found the code structure here somewhat confusing. I think it would be more obvious to write this as two separate checks:

```
// Normal constant int.
if (auto *CI = dyn_cast<ConstantInt>(V))
  return CI;

// Only handle constant integral pointers in the following.
if (!isa<Constant>(V) || !V->getType()->isPointerTy() ||
    DL.isNonIntegralPointerType(V->getType()))
  return nullptr;
```


================
Comment at: llvm/test/Transforms/SimplifyCFG/nonintegral.ll:5
+
+define void @test_01(i64 addrspace(1)* align 8 %ptr) local_unnamed_addr #0 {
+; CHECK-LABEL: @test_01(
----------------
The `local_unnamed_addr #0` bit can be dropped.


================
Comment at: llvm/test/Transforms/SimplifyCFG/nonintegral.ll:2
+; RUN: opt -simplifycfg -verify -S < %s | FileCheck %s
+; RUN: opt -passes=simplifycfg,verify -S < %s | FileCheck %s
+
----------------
vtjnash wrote:
> nikic wrote:
> > Explicit verify is unnecessary. First RUN line can be dropped, they do the same thing. Please use update_test_checks.py.
> I used update_test_checks.py to generate these tests, but disliked the results, and edited them to the form here. Has policy changed such that it is now required to be used verbatim?
Yes, auto-generated check lines are preferred. If you want to make it more explicit what this test is about, maybe add a comment?

```
; Check that ptrtoint is not produced for non-integral address spaces.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128670