Allow 32-bit pointers to be written in 64-bit slots

[dmlloyd]

Summary

Pointer lowering will presently happily allow the assembler to truncate a pointer in order to fit it into a narrower slot. The assembler is also capable of extending a pointer into a larger slot, and this is needed for languages like Java which may initialize a 64-bit integer field with a 32-bit pointer value on the initial heap.

This change builds on D61325 to allow the assembler to also extend pointers into larger integer slots.

Fixes #60453.

[dmlloyd]

I don't understand the build failure. clang-format was seemingly happy with the input, and I was able to build locally.

[arsenm]

test/CodeGen/WebAssembly/ptrtoint-widen.ll

5

Can you include some tests with different sized result integers? I thought that was the point of the patch

[dmlloyd]

test/CodeGen/WebAssembly/ptrtoint-widen.ll

5

For these two targets, the native pointer size is 32 bits, so i64 *is* the different-sized result. But I can also add in the native size and a shorter one for good measure. I will look around other tests to figure out how the CHECK would work in those cases.

[dmlloyd]

test/CodeGen/WebAssembly/ptrtoint-widen.ll

5

Well, it looks like there already is a test for narrowing a 64-bit pointer to 32 bits (llvm/test/CodeGen/X86/ptrtoint-narrow.ll for example), and plain ptrtoint is tested by other tests. But what I didn't realize before was that there is a test that specifically tests the inverse to the behavior I just changed, so I'll remove that and update the diff.

[dmlloyd]

Updated to remove a conflicting test.

[dmlloyd]

Another attempt to quell the build failures that I cannot reproduce locally.

[dmlloyd]

The CI system apparently does not like git diffs with --no-prefix.

[dmlloyd]

I think all concerns are now addressed and the build seems happy now. Did my explanation of the test situation make sense @arsenm?

[arsenm]

llvm/test/CodeGen/X86/ptrtoint-widen.ll

4 ↗

(On Diff #496152)

I still think it’s worthwhile to have a variety of destination integer sizes in the same test

[dmlloyd]

Updated to test more integer sizes per @arsenm.

[efriedma]

I'm concerned that this is going to emit expressions that the assembler can't actually handle. ".quad a" gives an error on many targets, including ARM and 32-bit x86.

[dmlloyd]

".quad a" gives an error on many targets, including ARM and 32-bit x86.

My first test case was 32-bit x86 so I can confirm that this works, however you are otherwise correct. I did some detailed manual testing and in addition to 64-bit platforms (obviously), it also works on these (presumably) non-64-bit targets, which all appear to have syntax for writing a 64-bit value (I am not familiar with all of these so forgive me for any errors):

• aarch64_32
• avr
• i686
• loongarch32
• mips & mipsel
• riscv32
• wasm32

However it does fail with "Don't know how to emit this value" on these platforms:

• arm & armeb
• ppc32 & ppc32le
• sparc & sparcel
• thumb & thumbeb

Testing with plain integers in these cases reveals that they normally emit a 64-bit constant as a pair of 32-bit words (usually .long).

So I'm thinking that there is some logic which knows to lower i64 into two distinct values in endian-correct order. Thus my plan is now to locate this information and figure out a way to use it on these platforms to split and zero-extend the lowered constant appropriately.

[dmlloyd]

An alternative plan would be to error as before on these platforms. It's trading one error for another but it'd still be an improvement from my perspective at least.

[efriedma]

Not sure how you're building i686, but I definitely get an error. (https://godbolt.org/z/MW3x3WKrr)

On the targets where it's working, I see 64-bit relocations (R_WASM_MEMORY_ADDR_I64, R_RISCV_64, R_MIPS_64), which might work with some linkers, but maybe not all.

I'd prefer to just explicitly pad values in the AsmPrinter, and avoid depending on obscure assembler behaviors.

[arsenm]

In D143602#4116836, @efriedma wrote:

I'd prefer to just explicitly pad values in the AsmPrinter, and avoid depending on obscure assembler behaviors.

Is that really worse than having the backend error?

[efriedma]

We have the following options here:

1. Leave it as, i.e. a backend error
2. Commit this as-is, i.e. depending on the target, it might work, but it could also either crash in the assembler or generate an invalid relocation.
3. Teach the assembler to emit padding for .quad.
4. Emit explicit padding in the AsmPrinter

(2) seems like the worst of these options.

[dmlloyd]

In D143602#4481817, @efriedma wrote:

We have the following options here:

1. Leave it as, i.e. a backend error
2. Commit this as-is, i.e. depending on the target, it might work, but it could also either crash in the assembler or generate an invalid relocation.
3. Teach the assembler to emit padding for .quad.
4. Emit explicit padding in the AsmPrinter

(2) seems like the worst of these options.

As the original author, I agree; when I wrote this I had wrongly assumed that all assemblers already supported this. I wouldn't mind if (3) or (4) were done but this would likely be beyond my experience with C++.

[arsenm]

moving out of needs review given there's an apparent desire to do something else here

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

case Instruction::IntToPtr: {
     // Handle casts to pointers by changing them into casts to the appropriate
     // integer type.  This promotes constant folding and simplifies this code.
     Constant *Op = CE->getOperand(0);
     Op = ConstantExpr::getIntegerCast(Op, DL.getIntPtrType(CV->getType()),
                                       false/*ZExt*/);
     return lowerConstant(Op);
   }
 
-  case Instruction::PtrToInt: {
-    const DataLayout &DL = getDataLayout();
-
+  case Instruction::PtrToInt:
     // Support only foldable casts to/from pointers that can be eliminated by
     // changing the pointer to the appropriately sized integer type.
-    Constant *Op = CE->getOperand(0);
-    Type *Ty = CE->getType();
-
-    const MCExpr *OpExpr = lowerConstant(Op);
-
+    //
     // We can emit the pointer value into this slot if the slot is an
-    // integer slot equal to the size of the pointer.
+    // integer slot.
     //
-    // If the pointer is larger than the resultant integer, then
-    // as with Trunc just depend on the assembler to truncate it.
-    if (DL.getTypeAllocSize(Ty).getFixedValue() <=
-        DL.getTypeAllocSize(Op->getType()).getFixedValue())
-      return OpExpr;
-
-    break; // Error
-  }
+    // If the pointer is larger or smaller than the resultant integer, then
+    // just depend on the assembler to truncate or extend it.
+    return lowerConstant(CE->getOperand(0));
 
   case Instruction::Sub: {
     GlobalValue *LHSGV;
     APInt LHSOffset;
     DSOLocalEquivalent *DSOEquiv;
     if (IsConstantOffsetFromGlobal(CE->getOperand(0), LHSGV, LHSOffset,
                                    getDataLayout(), &DSOEquiv)) {
       GlobalValue *RHSGV;

test/CodeGen/WebAssembly/ptrtoint-widen.ll

+; RUN: llc < %s -mtriple=wasm32-unknown-unknown | FileCheck %s
+
+@ptr = external global i8, align 1
+@ref = constant i64 ptrtoint (ptr @ptr to i64), align 8
+
+; CHECK: .int64  ptr{{$}}

test/CodeGen/X86/ptrtoint-constexpr-invalid.ll

-; RUN: not --crash llc < %s -mtriple=i386-linux 2>&1 | FileCheck %s
-
-; ptrtoint expressions that cast to a wider integer type are not supported.
-; A frontend can achieve a similar result by casting to the correct integer
-; type and explicitly zeroing any additional bytes.
-; { i32, i32 } { i32 ptrtoint (ptr @r to i32), i32 0 }
-
-; CHECK: LLVM ERROR: Unsupported expression in static initializer: ptrtoint (ptr @r to i64)
-
-@r = global i64 ptrtoint (ptr @r to i64)

test/CodeGen/X86/ptrtoint-widen.ll

+; RUN: llc < %s -mtriple=i686-unknown-linux-gnu | FileCheck %s
+
+@ptr = external global i8, align 1
+@ref = constant i64 ptrtoint (ptr @ptr to i64), align 8
+
+; CHECK: .quad  ptr{{$}}