We found an odd case in our downstream fork after this patch landed. We have a diagnostic in CheckVariableDeclarationType that checks for automatic variables that are too large for the stack address space, and it chokes on the testcase Parser/objcxx11-invalid-lambda.cpp:

So, any more on this or are we in agreement?

In D71550#1885169, @leonardchan wrote:

Not being able to do s.63 saturating multiplication kind of leaves a bad taste in my mouth, but for the purposes of implementing the standard, *I don't think* there's an instance where division for the proposed types can result in this (at least on a typical desktop processor). The main issues I can see down the road are frontend related, like if we perhaps will want to add something like long long _Fract that has a signed 64 bit width, or other frontends want to have types of varying scale and width.

Rebase and fix a few places where the FuncUnits type was missing.

Ah I see. Could you clarify though on how handling MIN / -EPS invokes undefined behavior?

In D71550#1835003, @leonardchan wrote:

It seems that I run into LLVM ERROR: Unsupported library call operation! if I call llvm.sdiv.fix.sat.i64 with a scale of 63:

define i64 @sdiv_sat_i64_s63(i64 %x, i64 %y) {
  %tmp = call i64 @llvm.sdiv.fix.sat.i64(i64 %x, i64 %y, i32 63)
  ret i64 %tmp;
}

This seems to be the only issue I could find from running the code.

Rebased.

Added tests for saturating types. Changed the warning group name for consistency.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Rebased.

Move packing of FixedPointSemantics to a separate patch.

Fixed typo.

Rebased.

Rebased.

Ping.

When promoting to a legal type, we must shift up the value for saturating divisions.

No need to do the promotion in DAGBuilder for unsigned saturating 0-scale operations, they cannot experience overflow.

I've been holding off on this for a while because the design made implementing saturated division a lot more complicated than it ought to be.

Add a typedef to InstrStage, FuncUnits, and use it as the bitmask type for functional units.

Maybe @craig.topper has some input on this design.

In D71210#1776911, @jmolloy wrote:

In fact, one concrete improvement we could do with this CL (which clearly found everything we needed to patch) is introduce a typedef for "A bitmask of functional units". This would allow us to find all the instances again in future, and highlight the point of the bitwidth to users.

I think I'd like someone who has more specialized SelectionDAG knowledge to come with some input as well, but it doesn't seem like anyone else is looking at the patch.

Fix review comments.

Sorry for the very late response on this. Hope it's not completely off your radar.

Ping.

Updated langref.

For what it's worth, in our downstream fork of Clang we have added the ability for function types to possess an address space.

I will preface this by saying that I'm not terribly pleased with this solution. There is no (easy) way to construct a wide division during operation legalization if the wider type is not legal, so we have to do a whole bunch of work upfront to avoid having to expand. It feels really hacky, but I'm not sure there's any other way of doing it given ISelDAG's design.

@fhahn I don't have commit access so if someone could land it for me, that would be great!

I don't have commit access, so it would be great if you could commit this for me.

Sorry for the very late response to this!

Rebased and addressed some comments.

In D62574#1552220, @Anastasia wrote:

Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.

Replaced compatiblyIncludes and its dependents with ASTContext accessors instead.

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?

In D62574#1523835, @Anastasia wrote:

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

This was accepted a while ago, but never landed. I don't have commit access; could someone commit it?

In D58236#1467151, @Anastasia wrote:

In D58236#1466263, @ebevhan wrote:

Well, it doesn't seem to me like there is consensus on prohibiting nested address space conversion like this.

I can simply redo the patch to only include the bugfix on implicit conversions and drop the nesting level checks.

I thought the conclusion is:

• Explicit conversions allowed for nested pointers with a warning.
• Implicit conversions are disallowed for nested pointers with an error.
  
  Do you not see it this way?

Well, it doesn't seem to me like there is consensus on prohibiting nested address space conversion like this.

In D57464#1438213, @Anastasia wrote:

I think I would lean towards the latter since it means less fudging around with a whole bunch of unrelated methods. Do @rjmccall or @rsmith have any further opinions on this?

Ok, I can change the patch to prototype this approach. I might need some example test cases though.

In D58236#1443082, @rjmccall wrote:

I think C probably requires us to allow this under an explicit cast, but we can at least warn about it. It does not require us to allow this as an implicit conversion, and that should be an error.

Any more input on this?

In D57464#1425904, @Anastasia wrote:

I think not. :( But I am wondering if we could proceed for now in some general direction and then make any improvements later. Probably the biggest part of this patch is not going to change. Would this make sense?

Any more input on this?

In D58236#1412267, @Anastasia wrote:

LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at generalizing this to C++ as well?