Hi! We're seeing miscompiles downstream when running attributor-light in pipelines. In the below reproducer, DSE is eliminating a store that isn't dead.

In D151087#4362059, @aaron.ballman wrote:

Based on all this, I think we should go with __addrspace_cast as a named cast and not allow the conversion through reinterpret_cast unless going to/from a [u]intptr_t.

In D151087#4361263, @jhuber6 wrote:

I'd rather have an operation whose semantics are a little dangerous than something that doesn't work at all. As it stands we need to use C-style casts for this and I don't think there's a good reason to forbid this at least from the C++ standard point of view. For OpenCL where we have the concept of address spaces it makes sense, but not for C++.

In D151087#4360859, @jhuber6 wrote:

You can have address spaced in freestanding C++, they just need to be assigned according to the backens, e.g. https://godbolt.org/z/ahazae6Ta. And I don't think that's a desirable solution because it would require a language extension to cover a use-case that isn't disallowed by the C++ standard anyway. I don't see a reason why we shouldn't be able to do this directly.

In D151087#4360743, @jhuber6 wrote:

The problem is we don't have addrspace_cast in freestanding C++, so as it stands we currently have no way to perform this operation in C++ which is preventing me from implementing things in the LLVM LibC port for GPUs I'm working on.

In D151087#4360606, @jhuber6 wrote:

In D151087#4360577, @ebevhan wrote:

What would be the semantics of such an operation if the address spaces are disjoint? Or, if the underlying pointer widths aren't the same?

It would most likely invalid, but I'm not asserting that clang should be responsible for diagnosing misuse in these cases. Especially because in generic freestanding C++ we don't have any language options to suggest the actual semantics.

What would be the semantics of such an operation if the address spaces are disjoint? Or, if the underlying pointer widths aren't the same?

In D58346#4359291, @jhuber6 wrote:

should C++ really be limited by OpenCL here?

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

We get a memory corruption issue in our downstream testing with this patch. I've attached a reproducer, which can be run with opt -passes=vector-combine reduced.ll:

Sorry for the delay! I rebased the patch onto latest and it mostly worked, with a few hitches.

Rebased and addressed some comments.

Hi! This test is failing in some of our downstream builds.

I've been away from LLVM development for a while, and sadly this patch has languished a bit.

It doesn't seem like anyone has any more immediate feedback. @efriedma, have all of your review comments been adequately addressed?

LGTM, but @aaron.ballman should probably give his two cents as well for completion's sake.

Rebased.

Rebased.

Ping. The build failures don't seem related to the patch.

Another gentle ping.

So is this patch good to go then?

Ping.

Ping. Any further comments?

Any more feedback on this?

Fixed typos.

Fix clang-format warnings.

Fix clang-format warnings.

Now only uses FP_TO_INT_SAT nodes for lowering. The nodes are selected in isel.

Fix typo.

Using Type::getFloatingPointTy now.

Use default expansion for f128 conversions; they aren't native so we can't be sure that they saturate.

Removed NativeSaturation parameter and made all lowering happen in Lower.

Redid lowering.

Followup to removing vector expansion in the parent patch.

Removed vector expansion.

Fixed failing test cases.

In D86078#2260930, @paulwalker-arm wrote:

I would have experimented a little to understand better how things work, but I'm guessing the patch is based on other work because I couldn't some of the affected functions/nodes.

Updated method name.

Addressed comments.

Removed superfluous NaN check and updated tests.

I don't know why the premerge tests are failing. I don't see those failures locally, so it's rather hard to debug it, especially since the premerge build lacks proper debug info.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Added promotion mechanism.

Added a promotion mechanism to handle cases where the floating point type cannot be used to rescale the value.

In D85961#2255847, @rjmccall wrote:

Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?

You could have the same problem with float/bfloat and a 128-bit fixed-point type, right? 128-bit integer types aren't at all ridiculous. I think this is something you should accommodate properly in your design; I don't think it's that problematic.

In D62574#2242471, @Anastasia wrote:

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at FakeAddrSpaceMap in LangOpts.

Made natively saturating expansions use integer comparisons instead.

In D54749#2242802, @efriedma wrote:

At least for the IEEE formats, you should be able to just destructure the bit-pattern of the float, right? Normalize and do some extends and shifts.

If we're going down that path, maybe simpler to just mess with the float scaling factor. If a float is close to the largest finite float, in IEEE formats, the fractional part must be zero. So we can do the float-to-int conversion on the unscaled float, and scale the resulting integer. Probably faster than trying to explicitly destructure a float.

Addressed comments.

I hadn't considered half precision as that's sort of off my radar. That does make both these conversion methods and the corresponding codegen implementation rather problematic.

In D54749#2239801, @rjmccall wrote:

I can't competently review LLVM code-gen patches, but I can review intrinsic design. If the intended use case here is fixed point, should there be a scale parameter rather than expecting the caller to scale the input? Or is scaling the input basically the expected implementation on all hardware? Scaling the input can be lossy if we overflow to infinity, so that would be making a tacit assumption here that if that happens, the original value must've been outside the representation range of the integer. For float (and bfloat), the exponent goes up to 127, so you would need to be converting to a pretty large fixed-point format to have problems with lossy overflow. For half, the exponent only goes up to 16, so converting to a 32-bit fixed-point format with a scale could overflow, and you probably won't be able to use these intrinsics.

Fix comment mistake.

This needs a few tests.

Rebased and did minor adjustments.

In D54749#2238740, @arsenm wrote:

Missing globalisel support

Ping.

In D86078#2233560, @paulwalker-arm wrote:

Given you are making use of the fact that AArch64 fp_to_int already does the saturation part, I'm wondering if the different-width saturation can be moved from the input to the output? Specially I'm thinking this will result in nicer code as the min/max immediate values will be easier to produce in integer form.

@gousemoodhin I just want to check; are you working on or planning to work on float-fixed support in the near future? Floating point conversions are the next step for the fixed-point support, so if you aren't immediately looking at it, I would like to take a shot at it.