In D114533#3153924, @jrtc27 wrote:

This seems like it should not apply to non-integral address spaces?

No, it shouldn’t care about the individual address spaces. It’s a reinterpret of the bits, the type meanings don’t matter

In D114533#3151482, @lebedev.ri wrote:

In D114533#3151423, @arsenm wrote:

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

That is a good point, but all the motivational cases seem to be about chaining the address space of the pointee pointer.
Does this come up for changing the address space of the pointer itself? I'm just wondering if this is still relevant with opaque pointers.

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

LGTM. Solving constant regbankselect is not really related and shouldn't hold this up

273a0c8bc9c774aa0d5982c23dc3d62b68ef4338

This makes my assert problem go away

LGTM, I've meant to do this before

A dedicated test (maybe MIR) would be nice

ping

In D110257#3134001, @JonChesterfield wrote:

So you won't articulate or document the new invariant and you think there's a llvm-dev discussion that says we can't verify the invariant which you won't reference, but means you won't add this to the verifier.

In D110257#3133866, @hsmhsm wrote:

This is not something specific to AMDGPU backend, but AMDGPU backend at present requires this canonical form.

GlobalISel version?

I'm still not a huge fan of purely negative checks since it's so easy for them to break

659887b4056236d376c0ac1218ca3f7a0dd75604

Last time I thought about this I thought it would be easier to have the post-regbank combiner handle this. In some situations it makes most sense to completely rematerialize the constant value in each regbank, not just reassign it. If you have an inline constant, it would be better to just emit a new constant for each bank. For multiple uses of literals its trickier since there's a code size or instruction count tradeoff based on the uses

In D113778#3127670, @foad wrote:

In D113778#3127658, @arsenm wrote:

64-bit shifts were quarter rate instructions last I checked, so this is slower

The Write64Bit definitions in SISchedule.td suggest they are half rate on most subtargets and full rate on gfx90a.

64-bit shifts were quarter rate instructions last I checked, so this is slower

I don't see the difference in the example, but maybe phabricator just ate the whitespace differences

In D112696#3124156, @tschuett wrote:

Do you have any users of the analysis or any plans? Otherwise, it could be seen as dead code?

In D112488#3122779, @arsenm wrote:

In D112488#3121906, @JonChesterfield wrote:

Can we land this? Seems likely to step on many latent bugs all at once

The only holdup is surviving openmp precheckin, which is mostly complicated due to a reverted commit

c7a0c2d0f7be2f456bd72b5c3508966d5b10233b

Description missing justification

In D112488#3121906, @JonChesterfield wrote:

Can we land this? Seems likely to step on many latent bugs all at once

Also test non-kernel

In D113538#3121062, @JonChesterfield wrote:

That seems important. What was the symptom of failing to set these? We may now be redundantly setting some, e.g.
I think convergent is set somewhere else before this patch.

90ff14871904881fb156a1d4d5fb083ca75998ab

ping

Is this still relevant? We want to move towards consistently using byref for kernel arguments anyway

Is this still relevant? I thought this was done already

You're probably experiencing a driver issue

ping

In D112760#3101240, @andrew.w.kaylor wrote:

In D112760#3097718, @arsenm wrote:

I would rephrase the description as removing the global flag for contraction

This change also removes the behavior of the function attribute "unsafe=fp-math" enabling fp-contraction.

In D109032#3020826, @jrtc27 wrote:

The MUBUFAddr64 case isn't that interesting, since you're changing one form of "i64" to another form of "i64". I'm much more puzzled about how this is not blowing up on the cases that do use 32 bit pointers (e.g. all the DS* patterns)

Those examples may not be what you intend them to be, but I can promise you that if you go read AMDGPUGenDAGISel.inc yourself you will find that every single one of them is currently matching an iPTR. For example:

// Src: (ld:{ *:[i32] } (ScratchOffset:{ *:[iPTR] } VGPR_32:{ *:[i32] }:$vaddr, i16:{ *:[i16] }:$offset))<<P:Predicate_unindexedload>><<P:Predicate_extload>><<P:Predicate_extloadi8_private>> - Complexity = 22
// Dst: (SCRATCH_LOAD_UBYTE:{ *:[i32] } ?:{ *:[i32] }:$vaddr, ?:{ *:[i16] }:$offset)

is what I see *without* any of my patches, so I'm just preserving that. Things blow up with failures to select, or poor codegen that regresses tests, when these instead infer i32. If you want to see for yourself, apply D109035, note that the TableGen output changes to honour the existing intended types and that many AMDGPU tests fail.

Ping regarding this explanation?

I would rephrase the description as removing the global flag for contraction

In D112644#3091224, @rampitec wrote:

In D112644#3091209, @arsenm wrote:

In D112644#3091088, @rampitec wrote:

In D112644#3091054, @arsenm wrote:

How was this breaking?

LLVM ERROR: no registers from class available to allocate

What happens usesAGPRs() does not see any agprs used and then the whole register budget is allocated to vgprs.

Wouldn't it work correctly after later getReservedRegs calls? I don't think we should be calling this before finalizeIsel?

It does work correctly with a later call from freezeReservedRegs(). The first call though comes from MachineVerifier::visitMachineFunctionBefore().

We should also apply these pre-legalization like in the DAG

52fc2edb5357075c7c746adc274d513f48d412b8 plus rebase