Oh, sorry. I should have mentioned that. That would be nice, thanks!

So if I understand this correctly, we can handle this intrinsic in 2 ways:

1. The intrinsic gets converted to IR in a pass before touching the DAG.
2. The intrinsic gets passed to the DAG and gets legalized and lowered to machine code there.

It seems that from the thread, the consensus was for going with (2), but I also see now how things would be much easier just having this run through a pass in (1). If I recall correctly, the main reason for offsetting the intrinsic "expansion" to the DAG was to determine if the intrinsic was supported by the hardware during the legalization steps. If so, couldn't this also be done in a pass when converting the intrinsic to IR in (1)? That is, we determine early in the pass if this intrinsic call is supported by this target. Those that aren't legal get expanded into IR whereas those that are legal get passed to DAG instruction selection.

I think this might avoid the trouble of having to check types and operations in the DAG, and ISel can choose whatever appropriate instructions for whatever supported intrinsic (if that makes sense).

Another ping. Anyone up for reviewing this patch?

I think we need to decide where these intrinsics should be 'lowered' if they aren't legal, and define how to let the target specify legality.

Ping.

I don't have commit access, so it would be appreciated if someone could commit it.

Added Embedded-C clause quote.
Fixed formatting.

In D50616#1204588, @leonardchan wrote:

Would it be simpler instead just to have the logic contained in the virtual function for TargetCodeGenInfo as opposed to returning nullptr since any custom target will end up overriding it anyway and ideally not return nullptr?

I guess that's also possible. I figured it would be clearer to have the generic logic in the more obvious location (CGExprScalar) and fall back to the custom handling if needed. Many of the handlers in TargetCodeGenInfo are empty.

In D50616#1203446, @leonardchan wrote:

Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the EmitFixedPointConversion function should be moved to a virtual method under TargetCodeGenInfo that can be overridden and this is what get's called instead during conversion?

Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.

I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.

I think the solution to a lot of diagnostic and behavior issues with address spaces is to remove predication on OpenCL. It's a bit odd to have a language feature that is enabled out of the box regardless of langoptions (address spaces) but won't actually work properly unless you're building OpenCL.

When I try the test case on our downstream (and when compiling for our target with an extra flag that enables a bunch of OpenCL-related address space code), I get:

ascrash.c:5:12: error: comparison between ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which
      are pointers to non-overlapping address spaces
  return x < y ? x : y;
         ~ ^ ~
ascrash.c:5:16: error: conditional operator with the second and third operands of type ('__attribute__((address_space(1))) char *' and
      '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces
  return x < y ? x : y;
               ^ ~   ~

A lot of address space code is hidden behind LangOptions.OpenCL.

LGTM.

Sorry for the late response, I've been away.

I also think it would be good with some unit tests for this class once the functionality and interface is nailed down.

Ping.

Would it be possible to add some form of target hook (perhaps to CodeGenABIInfo, which is already accessed with getTargetHooks) for fixed-point operations (maybe even some conversions)? As I've mentioned earlier, we emit both IR and intrinsics for many of these operations to make instruction selection possible. I suspect that any target that wants to utilize fixed-point effectively will need this as well.

Thanks! I do not have commit access, so it would be great if someone could commit this.

Just a couple more comments and then I think it looks good.

LGTM, but I'm not a code owner so maybe someone else should chime in if they have any input.

Well, the documentation mismatch is worth fixing even if the code isn't. But I think at best your use-case calls for weakening the assertion to be that any existing address space isn't *different*, yeah.

Added a warning for identical address spaces.

Actually, wait! One last thing I missed.

Yes, it looks good to me.

There's actually a bit more to it than in these two patches. The complete diff to this function in our downstream Clang looks like this:

 QualType
 ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const {
-  QualType CanT = getCanonicalType(T);
-  if (CanT.getAddressSpace() == AddressSpace)
+  if (T.getLocalQualifiers().getAddressSpace() == AddressSpace)
     return T;

I have this patch uploaded as well: https://reviews.llvm.org/D47630

Changed ArrayIndexTy back to LongLongTy and reverted the test change.

Sorry for the late notice; I missed something.

With the exception of the two inline comments, this looks good to me now!

Also, this patch and all of the following 'Validation Test' patches do much more than just add tests, they add plenty of functionality as well. In general, it's quite difficult to tell which patches add what.

I cannot say that I'm pleased with the CodeGen emission of the operations as pure IR. I can only assume that you do not have hardware specifically tailored for these operations, as matching this type of code ought to be quite difficult after optimization is performed.

You should not define the fixed-point precision as compiler macros at build configure time. The number of fractional bits (the scaling factor) should be added to TargetInfo as target-configurable variables/accessors, and an accessor should be added to ASTContext (we call it 'getFixedPointScale') to fetch the scaling factor of arbitrary fixed-point types.

Made ArrayIndexTy into ssize_t, consolidated the tests and fixed the test that was failing.

Thank you! I do not have commit rights, so if someone could commit this that would be great.

Removed local variable and added some more to the test.

Rebased. Another test case was affected, but as far as I can tell, the only difference is the elimination of two bitcasts.

I replaced the other two instances of Ty with nullptr, and only observed a difference in one AMDGPU test case (preserve-addrspace-assert.ll), likely caused by the change on line 5022. Before, GEPs for double* and i32* were produced, but with the patch, GEPs for the containing struct are produced instead. This is probably because the expander was being forced to emit the pre-offset expressions as the final types (which were double* and i32*) but with the patch it will emit GEPs that take the struct pointer instead.

In D42103#989687, @qcolombet wrote:

Out-of-curiosity do you actually see codegen differences with that patch?

Included the complete context. Sorry for missing that!