[SelectionDAG] Skip addrspacecast expansion when casting undef values

[dylanmckay]

Summary

This allows all targets to expand address space casts of undef values,
even if they do not explicitly have NOP addrspacecasts.

This bug was encountered whilst compiling the Rust libcore library for
AVR.

Prior to this, only targets that defined isNoopAddrSpaceCast() as true
could successfully lower an undef address space cast without custom
lowering logic.

The Mips and PowerPC backends, among others, could already handle these
addrspacecasts fine because SelectionDAG knew all addrspacecasts were NOPs.
Other backends including MSP430 and AVR would crash during ISel.
Now all backends will correctly lower this form of instruction without special
consideration from the backend authors.

[dylanmckay]

Remove junk comment line

[arsenm]

I don't really understand the point of this. It seems more like a workaround

[dylanmckay]

Thank you for looking over this @arsenm.

In my particular case, the IR that was triggering the failure used an undef value. Now that you mention it, I can see that perhaps the undef was coincidental and the current fix leaves the general case of any addrspacecast unhandled.

After reading the LangRef docs on addrspacecast, this line stuck out:

Note that if the address space conversion is legal then both result and operand refer to the same memory location.

On AVR, there is no transformation you can do to a pointer to transform it into a pointer in a different address space, that still points at the same cells in memory - there is no MMU, paging, virtual memory or memory mapping (outside of IO registers and occasionally, EEPROMS). If you want to load from program space, you must use lpm, if you want to load from data space, you must use ld.

Does this mean that addrspacecasts should be illegal on AVR and not emitted by the frontend? Feels strange to have a backend silently supporting only a subset of the full IR gamut - perhaps the GPU backends do this too?

At the moment, the AVR backend does not handle addrspacecasts at all, always hitting an assertion during ISel.

What do you think would be the way forward?

[efriedma]

If you look at some targets, they provide multiple address-spaces which are essentially identical; see, for example, the implementation of isNoopAddrSpaceCast on x86. Not sure what the complete justification was, but it can be convenient if you need to dereference null.

It's legitimate for a target to reject addrspacecasts which don't make sense.

[efriedma]

It's legitimate for a target to reject addrspacecasts which don't make sense.

Actually, I'm not sure this is right. Clearly, if the underlying memory doesn't overlap, the result doesn't have to make sense. This is a fundamental part of supporting overlapping address-spaces. But actually rejecting it might impose some unexpected restrictions on optimizations (e.g. whether it's possible to combine two addrspace casts), so maybe it should actually just return undef? LangRef currently doesn't say; probably worth clarifying.

This bug was encountered whilst compiling the Rust libcore library for AVR.

I'm guessing blindly, but given the conversion doesn't produce a useful result on AVR, this probably indicates a bug in the Rust compiler.

[arsenm]

In D62775#1528629, @dylanmckay wrote:

Thank you for looking over this @arsenm.

In my particular case, the IR that was triggering the failure used an undef value. Now that you mention it, I can see that perhaps the undef was coincidental and the current fix leaves the general case of any addrspacecast unhandled.

Yes, I don't see the point of special casing avoiding a need to handle undef. A target may want to do something special for its undef fold I guess? If you aren't handling the entire operation, there's not much benefit to just undef working.

After reading the LangRef docs on addrspacecast, this line stuck out:

Note that if the address space conversion is legal then both result and operand refer to the same memory location.

On AVR, there is no transformation you can do to a pointer to transform it into a pointer in a different address space, that still points at the same cells in memory - there is no MMU, paging, virtual memory or memory mapping (outside of IO registers and occasionally, EEPROMS). If you want to load from program space, you must use lpm, if you want to load from data space, you must use ld.

Does this mean that addrspacecasts should be illegal on AVR and not emitted by the frontend? Feels strange to have a backend silently supporting only a subset of the full IR gamut - perhaps the GPU backends do this too?

At the moment, the AVR backend does not handle addrspacecasts at all, always hitting an assertion during ISel.

What do you think would be the way forward?

For AMDGPU, we custom lower addrspacecast and emit an error for the illegal ones: https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AMDGPU/SIISelLowering.cpp#L4396

[dylanmckay]

Good point @arsenm and @efriedma, I agree that the problem lies elsewhere. If I remember correctly, this actually was a bug in the Rust compiler (at least for AVR), and we worked around it by slightly rewriting a function pointer cast in Rust's libcore that triggered the address space information to be lost.

I will close this patch, thanks for the review time everyone.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitAddrSpaceCast(const User &I) {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   const Value *SV = I.getOperand(0);
   SDValue N = getValue(SV);
   EVT DestVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 
   unsigned SrcAS = SV->getType()->getPointerAddressSpace();
   unsigned DestAS = I.getType()->getPointerAddressSpace();
 
-  if (!TLI.isNoopAddrSpaceCast(SrcAS, DestAS))
+  // Do not insert address space conversion code when the target doesn't
+  // require it. Also skip cast expansion when casting an undefined
+  // value, as a casted undefined value is still an undefined value.
+  if (!TLI.isNoopAddrSpaceCast(SrcAS, DestAS) &&
+      !isa<UndefValue>(SV))
     N = DAG.getAddrSpaceCast(getCurSDLoc(), DestVT, N, SrcAS, DestAS);
 
   setValue(&I, N);
 }
 
 void SelectionDAGBuilder::visitInsertElement(const User &I) {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   SDValue InVec = getValue(I.getOperand(0));

test/CodeGen/AVR/undef-addrspacecast.ll

+; RUN: llc -mattr=avr6 < %s -march=avr | FileCheck %s
+
+; CHECK-LABEL: foo
+define void @foo() addrspace(1) {
+start:
+  %a = addrspacecast {} addrspace(1)* undef to {}*
+  store {}* %a, {}** undef, align 1
+  ret void
+}
+