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

[vtjnash]

Summary

SimplifyCFG expects to be able to cast both sides to an int, if either side can be case to an int, but this is not desirable or legal, in general, per D104547.

Spotted in https://github.com/JuliaLang/julia/issues/45702

[nikic]

llvm/test/Transforms/SimplifyCFG/nonintegral.ll

3

Explicit verify is unnecessary. First RUN line can be dropped, they do the same thing. Please use update_test_checks.py.

[vtjnash]

run tests once

llvm/test/Transforms/SimplifyCFG/nonintegral.ll

3

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?

[nikic]

LG

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

478

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;

llvm/test/Transforms/SimplifyCFG/nonintegral.ll

3

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.

6

The local_unnamed_addr #0 bit can be dropped.

[vtjnash]

llvm/test/Transforms/SimplifyCFG/nonintegral.ll

3

That seems to be at odds with both the testing guide (which implies it is optional) https://llvm.org/docs/TestingGuide.html#generating-assertions-in-regression-tests
and the tool itself (which states the results should be edited to remove extra lines, and the output is not designed to be representative of a good test):
https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/utils/update_test_checks.py#L27-L29

[vtjnash]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

478

I agree, if it was newly written, but also wasn't sure it warranted rearrangement, which would complicate the PR diff

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

static bool dominatesMergePoint(Value *V, BasicBlock *BB,
   return true;
 }
 
 /// Extract ConstantInt from value, looking through IntToPtr
 /// and PointerNullValue. Return NULL if value is not a constant int.
 static ConstantInt *GetConstantInt(Value *V, const DataLayout &DL) {
   // Normal constant int.
   ConstantInt *CI = dyn_cast<ConstantInt>(V);
-  if (CI || !isa<Constant>(V) || !V->getType()->isPointerTy())
+  if (CI || !isa<Constant>(V) || !V->getType()->isPointerTy() ||
+      DL.isNonIntegralPointerType(V->getType()))
     return CI;
 
   // This is some kind of pointer constant. Turn it into a pointer-sized
   // ConstantInt if possible.
   IntegerType *PtrTy = cast<IntegerType>(DL.getIntPtrType(V->getType()));
 
   // Null pointer means 0, see SelectionDAGBuilder::getValue(const Value*).
   if (isa<ConstantPointerNull>(V))
     return ConstantInt::get(PtrTy, 0);

llvm/test/Transforms/SimplifyCFG/nonintegral.ll

+; RUN: opt -passes=simplifycfg -S < %s | FileCheck %s
+
+target datalayout = "ni:1"
+
+define void @test_01(i64 addrspace(1)* align 8 %ptr) {
+; CHECK-LABEL: @test_01(
+; CHECK-NOT:   ptrtoint
+; CHECK-NEXT:  icmp eq i64 addrspace(1)* %ptr, null
+; CHECK-NOT:   ptrtoint
+  %cond1 = icmp eq i64 addrspace(1)* %ptr, null
+  %cond2 = icmp eq i64 addrspace(1)* %ptr, null
+  br i1 %cond1, label %true1, label %false1
+
+true1:
+  br i1 %cond2, label %true2, label %false2
+
+false1:
+  store i64 1, i64 addrspace(1)* %ptr, align 8
+  br label %true1
+
+true2:
+  store i64 2, i64 addrspace(1)* %ptr, align 8
+  ret void
+
+false2:
+  store i64 3, i64 addrspace(1)* %ptr, align 8
+  ret void
+}