patch for missed constantFold optimization in InstCombine

[optimusPrime]

Summary

llvm::ConstantFoldCompareInstOperands misses an optimization when it tries to constantFold a compare instruction containing addrspacecast instead of combination of ptrtoint and inttoptr. Adding the patched code snippet in the code would enable the pass to handle addrspacecast and constantFold the missing cases.
Attached is the reproducer. Let me know if there are any suggestions/comments.

instCombineAddrspaceCast.ll1 KBDownload

[majnemer]

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

[optimusPrime]

In D14721#290414, @majnemer wrote:

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

Hi David,
I have already attached a test case in the summary section. Isn't that sufficient ?

[majnemer]

In D14721#290480, @optimusPrime wrote:

In D14721#290414, @majnemer wrote:

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

Hi David,
I have already attached a test case in the summary section. Isn't that sufficient ?

It should be part of the patch.

[optimusPrime]

Uploaded the testcase scenario in the patch itself instead of summary.

[optimusPrime]

Hi David,
Added the test case as a part of the patch as you suggested. Does this look okay ?

Thanks,
Tejas

[majnemer]

Please include additional context in the diff.

test/Transforms/InstCombine/instCombineOptimizeAddrspaceCast.ll

8–26 ↗

(On Diff #40553)

Can you try to make this test a little more targeted?

[optimusPrime]

Hi David,
Added some more context in both the .ll and cpp. I don't think the ll can be shortened more, this is the bare minimum needed to test.

Thanks

[meadori]

lib/Analysis/ConstantFolding.cpp

1140

Seems to be some extraneous spaces here.

1147

Is this patch against mainline trunk?

Invocations of ConstantFoldCompareInstOperands that I see look like:

ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);

[mgrang]

lib/Analysis/ConstantFolding.cpp

1141

Ideally it's best to submit the entire file containing your changes. I would do this as follows:

git diff <previous_commit_id> HEAD -U99999 > my.patch

1142

Maximize the 80 column line width here.

1147

Your patch seems to be based on an older version of code. In the latest tip the signature of ConstantFoldCompareInstOperands does not use "TD". It uses DataLayout (DL) instead:

return ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);

test/Transforms/InstCombine/instCombineOptimizeAddrspaceCast.ll

1 ↗

(On Diff #40807)

The test name does not need to contain "instCombine". It can simply be called OptimizeAddrspaceCast.ll

[optimusPrime]

Modified according to Reviewer's suggestions.

[optimusPrime]

Modified the space formatting according to llvm convention.

[optimusPrime]

Hi Reviewers,
Any comment on the updated version ?

[optimusPrime]

I have made all the changes suggested by David, Meador and Mandeep. Can I commit this change now ?

[majnemer]

Your test doesn't parse on trunk LLVM:
opt: <stdin>:14:32: error: expected comma after load's type

%2 = load i32 addrspace(4)** %ptr, align 8

[optimusPrime]

Hi David,
Corrected the test and tested against trunk. Can you take a look ?

[mkuper]

Can you please include additional context in the diff, as requested above?
(See http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for directions.)

lib/Analysis/ConstantFolding.cpp

1144

I'm not sure I understand why this is correct.

My understanding of the semantics of addrspacecast is that it may change the integer value of the pointer ("It can be a no-op cast or a complex value modification, depending on the target and the address space pair"). Since icmp on pointers compares integer values ("If the operands are pointer typed, the pointer values are compared as if they were integers."), this transformation may change the result of the comparison.

Am I missing something?

test/Transforms/InstCombine/OptimizeAddrspaceCast.ll

27

Any chance to reduce the test case? It looks very large, compared to what it's actually checking.

[optimusPrime]

Added additional context.

lib/Analysis/ConstantFolding.cpp

if (Ops1->isNullValue()) {
       if (CE0->getOpcode() == Instruction::PtrToInt) {
         Type *IntPtrTy = DL.getIntPtrType(CE0->getOperand(0)->getType());
         if (CE0->getType() == IntPtrTy) {
           Constant *C = CE0->getOperand(0);
           Constant *Null = Constant::getNullValue(C->getType());
           return ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);
         }
       }
+
+      // ConstantFold a compare instruction with addrspacecast as the
+      // first operand.
+      // fold: icmp (addrspacecast x), y     -> icmp x, y
+      if (CE0->getOpcode() == Instruction::AddrSpaceCast) {
+        Constant *C = CE0->getOperand(0);
+        Constant *Null = Constant::getNullValue(C->getType());
+        return ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);
+      }
     }
 
     if (ConstantExpr *CE1 = dyn_cast<ConstantExpr>(Ops1)) {
       if (CE0->getOpcode() == CE1->getOpcode()) {
         if (CE0->getOpcode() == Instruction::IntToPtr) {
           Type *IntPtrTy = DL.getIntPtrType(CE0->getType());
 
           // Convert the integer value to the right size to ensure we get the

test/Transforms/InstCombine/OptimizeAddrspaceCast.ll

+; Test to make sure that the addrspacecast is constantfolded correctly
+; during InstCombine pass.
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+target datalayout = "e-i64:64-f80:128-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-n8:16:32:64"
+
[e-mail]_value = internal addrspace(3) global i32 undef, align 4
+
+; Function Attrs: nounwind
+define void @testKernel(i32 addrspace(1)* %results) nounwind {
+  %1 = alloca i32 addrspace(1)*, align 8
+  %ptr = alloca i32 addrspace(4)*, align 8
+  %lptr = alloca i32 addrspace(3)*, align 8
+  store i32 addrspace(1)* %results, i32 addrspace(1)** %1, align 8
+  store i32 7, i32 addrspace(3)* @testKernel.some_value, align 4
+  store i32 addrspace(4)* null, i32 addrspace(4)** %ptr, align 8
+  store i32 addrspace(3)* @testKernel.some_value, i32 addrspace(3)** %lptr, align 8
+  %2 = load i32 addrspace(4)*, i32 addrspace(4)** %ptr, align 8
+  %3 = load i32 addrspace(3)*, i32 addrspace(3)** %lptr, align 8
+  %4 = addrspacecast i32 addrspace(3)* %3 to i32 addrspace(4)*
+  %5 = icmp ne i32 addrspace(4)* %2, %4
+  %6 = zext i1 %5 to i32
+  %7 = load i32 addrspace(1)*, i32 addrspace(1)** %1, align 8
+  %8 = getelementptr inbounds i32, i32 addrspace(1)* %7, i64 0
+  store i32 %6, i32 addrspace(1)* %8, align 4
+; CHECK: store i32 1, i32 addrspace(1)* %results
+  ret void
+}