In D52052#1236976, @qcolombet wrote:

My understanding is that you don't want split code to occur right before this instruction, but I would like to understand exactly what it is not allowed.

The amdgpu target has special exec mask register that specifies what lanes should execute current vector instruction. So a basic block created from "if/else" construction with vector condition may contain this exec mask register restore code in preamble.
E.g. instruction S_OR_SAVEEXEC_B64 at the start of a BB set (restores) exec mask register value to correctly execute "else" block. So a spilling code SplitKit tries to insert before such an instruction may be transformed into memory operation. And it can be incorrectly executed with a wrong exec mask.

In D52052#1235054, @qcolombet wrote:

Hi,

Could you elaborate on that part?

But it is not possible since can generate incorrect code in terms of exec mask.

I don't understand the constraints and thus what you are trying to achieve.

Cheers,
-Quentin

SplitEditor during splitting region can try to insert COPY spill instruction before S_OR_SAVEEXEC_B64 since it interferes with physreg candidate that is already assigned to destination of this exec mask restoration instruction. Unfortunately, such a COPY operation may be lowered to a memory operation that will not be correct because exec mask is not restored before the preamble.

In D48558#1142632, @kparzysz wrote:

The testcase compiles fine with ToT at r335473.

I have a quite small .ll that causes assertion in SIFrameLowering.cpp, if you need

The patch fixes the issue I found in Blender application

test updated as requested

ping...

ping...

diff updated as requested

ping...

My real question was what happens if you put 11 in the description string?

in this case CanT.getAddressSpace() returns target addrspace value "20" (also shifted in the enum by 9==LangAS::FirstTargetAddressSpace)

ping...

addrspace specifications are kept in descriptions strings

The problem is that if set addrspace "2" in description string, CanT.getAddressSpace() returns target addrspace value "11" (shifted in the enum) and compares it with input LangAS addrspace ("2", "opencl_local" in our case).
So I cannot set a number a description string that will be equal to LangAS addrspace "opencl_local".

In D43281#1018657, @arsenm wrote:

Can’t you just change the description to be the LangAS value? I also thought these happened to be the same already

ping...

diff updated as requested by reviewer

any test and/or example of the case?

fixed builtins descriptions

Sorry, missed them

diff updated according to latest comments

ping

updates as requested by reviewers

Sorry for incomplete diff

otherwise LGTM

Am I right that since we should have almost the same processing as atomic inc intrinsics,
it would be better idea to define ds_add/min/max intrinsics the same way as AMDGPUAtomicIncIntrin (or unify them),
and then update AMDGPU BE to correctly process these ds_ instrisics the same ways as atomic inc/dec?

Hi Sanjoy,

ping

ping...

1. add expr inlining limit moved to separate change https://reviews.llvm.org/D28812
2. removed depth parameter from wrapper functions
3. added helper function to early return
4. refined test to use func params instead of undef

ping...

What do you mean exactly? getMulExpr does not have a depth parameter like this. We have a comparison depth limit and a depth limit on SimplifyICmpOperands.

The second point is about new AddOpsInlineThreshold parameter. It is almost the same as MulOpsInlineThreshold used in getMulExpr (line 2608). And this part of change is independent of recursion depth limit, but suggested for the same purpose - to reduce almost infinite time of processing very long expressions.

refined with Chris suggestion

Hi Sanjoy, would you please check the updated diff and test?