This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Implement lower for G_BITCAST
ClosedPublic

Authored by arsenm on Jan 9 2020, 7:19 PM.

Details

Summary

Bitcast only really applies between scalars and vectors. Implement as
an unmerge and remerge. The test needs to tolerate failure since one
of the unmerges currently fails to legalize.

Diff Detail

Event Timeline

arsenm created this revision.Jan 9 2020, 7:19 PM
arsenm updated this revision to Diff 237235.Jan 9 2020, 7:21 PM

Remove leftover junk

arsenm updated this revision to Diff 237236.Jan 9 2020, 7:36 PM

Move to function

arsenm updated this revision to Diff 237237.Jan 9 2020, 7:43 PM

Remove leftovers

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

We don't have an alternative way to easily convert between them though. Unless G_PTRTOINT is also allowed to take vectors of pointers.

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

We don't have an alternative way to easily convert between them though. Unless G_PTRTOINT is also allowed to take vectors of pointers.

G_PTRTOINT works with vectors. I'm not opposed to allowing G_BITCAST between scalar and (integral) pointers, although this differs from the IR. It makes more sense in codegen to have these be more flexible. The current state doesn't make sense though. We can either:

  1. Allow casts between pointers and others types. This will need documenting as differing from the corresponding IR instructions. This should also maybe restrict this for nonintegral address spaces?
  1. Disallow bitcast between vectors of pointers and vectors of scalars and match the IR

On AArch64 we also use bitcast to convert from <n x p0> to <n x s64> types.

I thought that was illegal. It’s not legal in IR

I think this is a verifier bug. You're not allowed to bitcast between scalar pointers and scalars, so allowing it for vectors doesn't make much sense. I think allowing this is potentially problematic for non-integral pointers since it makes it easy to accidentally launder away the pointerness

We don't have an alternative way to easily convert between them though. Unless G_PTRTOINT is also allowed to take vectors of pointers.

G_PTRTOINT works with vectors. I'm not opposed to allowing G_BITCAST between scalar and (integral) pointers, although this differs from the IR. It makes more sense in codegen to have these be more flexible. The current state doesn't make sense though. We can either:

Ah, I didn't realize it was allowed in the IR. In that case AArch64 can move to using that instead.

  1. Allow casts between pointers and others types. This will need documenting as differing from the corresponding IR instructions. This should also maybe restrict this for nonintegral address spaces?
  1. Disallow bitcast between vectors of pointers and vectors of scalars and match the IR

Without a clear need maybe we should err on the conservative side and keep the semantics restricted and matching the IR when possible. If we have to revisit this in future we can do.

On the topic of this particular change: IIRC the semantics of bitcast is that it's equivalent of doing a store of the source type and a load of the dest type, which can have differing results on big endian targets. Is this lowering code safe in that case?

On the topic of this particular change: IIRC the semantics of bitcast is that it's equivalent of doing a store of the source type and a load of the dest type, which can have differing results on big endian targets. Is this lowering code safe in that case?

This doesn't touch memory, so the endianness shouldn't matter

aemerson accepted this revision.Jan 14 2020, 1:14 PM
This revision is now accepted and ready to land.Jan 14 2020, 1:14 PM