LGTM

LGTM

It looks good to me, but @arsenm could you also take a look?

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

Can we have a mir test with more than two loads? I want to see a situation where 3 loads are foldable with the same offset, but lowest address is in the middle. I.e.:

In D55539#1327264, @arsenm wrote:

In D55539#1327081, @FarhanaAleen wrote:

In D55539#1326636, @arsenm wrote:

Why aren't these matched in the first place? These shouldn't have gotten this far

The offsets are promoted to immediate during the instruction selection if they are 13bit which is the allowed size for globals. This patch is trying to find a 13bit offset from base-address of the nearby instructions by recomputing the relative offset from nearby base address. So, it's more of a global optimization where it needs to traverse the whole program (currently it's limited to the basic block). The optimization also caches all the information to save the compile time. Are you suggesting to do this optimization during instruction selection phase? Wouldn't it be too complex to do during instruction selection?

OK, so this is the case where the offset > 13-bits and you are folding the extra low bits. We already do something similar during selection and put the low bits into the immediate offsets and materialize the high bits as a separate constant for some other loads (e.g. see SelectMUBUFScratchOffen). Usually the common high bits get CSEd in the DAG directly or during MachineCSE. Do you get the same result if you extend that to global/flat operations for this?

Also can you reword the commit message to make it clearer what the difference is from the current offset matching?

LGTM

LGTM

Rebased.

Switched to MDT use.
Added tests with cross block defines.

Reduced test.

LGTM

LGTM

LGTM

LGTM

As far as I understand if PredicateCodeUsesOperands is not used nothing changes.
LGTM.

The error is that condition is actually has to be negated. However, if I just negate it it goes into an endless loop inside the combiner. I need to fix our BRCOND lowering instead.

It does not validate in some tests. I will investigate it as I do not see right away what is wrong.

Added sext handling.

In D55030#1312124, @arsenm wrote:

Should this be a generic combine?

In D54882#1310543, @ronlieb wrote:

In D54882#1309928, @rampitec wrote:

In D54882#1309888, @ronlieb wrote:

In D54882#1309711, @rampitec wrote:

In D54882#1309708, @ronlieb wrote:

Adding the shrink pass just before Peephole SDWA does not help the lit test, it made no difference.
I think at this point i should proceed with adding the MIR test to verify when we cannot fold.

Do you know why was it unable to shrink these instructions?

SIInstrInfo::splitScalar64BitAddSub converts the S_ADD_U64_PSEUDO into the two add instructions which use the SReg_64_XEXECRegClass instead of VCC.
Later when SIShrinkInstructions::runOnMachineFunction pass runs, it sees that the Carry regs are not VCC and simply marks them with a hint to later convert to VCC ,
and then continues without doing a transformation.

if (SDst) {
  if (SDst->getReg() != AMDGPU::VCC) {
    if (TargetRegisterInfo::isVirtualRegister(SDst->getReg()))
      MRI.setRegAllocationHint(SDst->getReg(), 0, AMDGPU::VCC);
    continue;
  }
 
  // All of the instructions with carry outs also have an SGPR input in
  // src2.
  if (Src2 && Src2->getReg() != AMDGPU::VCC) {
    if (TargetRegisterInfo::isVirtualRegister(Src2->getReg()))
      MRI.setRegAllocationHint(Src2->getReg(), 0, AMDGPU::VCC);
 
    continue;
  }
}

OK, that makes sense. It just leaves it to post-RA shrink that way. Probably shrink pass could do the same, but it is not desirable as it limits scheduling opportunities.

But I see the problem in your code now: you do not check that vcc is not clobbered or used in between of two instructions.
I also think you need to shrink both instructions, otherwise you have carry-in of addc and carry-out of add in different registers, which just happen to be allocated to the same vcc. Note, that isConvertibleToSDWA() returning true does not guarantee final sdwa conversion, so you can end up with vop3 form for the first instruction anyway.

i think the following code does make sure that there are no intervening uses, i can also strengthen it to make sure the defining instruction of the CarryIn is the first ADD instruction.
+ if (!MRI->hasOneUse(CarryIn->getReg()) || !MRI->use_empty(CarryOut->getReg()))
+ return false;

In D54882#1309888, @ronlieb wrote:

In D54882#1309711, @rampitec wrote:

In D54882#1309708, @ronlieb wrote:

Adding the shrink pass just before Peephole SDWA does not help the lit test, it made no difference.
I think at this point i should proceed with adding the MIR test to verify when we cannot fold.

Do you know why was it unable to shrink these instructions?

SIInstrInfo::splitScalar64BitAddSub converts the S_ADD_U64_PSEUDO into the two add instructions which use the SReg_64_XEXECRegClass instead of VCC.
Later when SIShrinkInstructions::runOnMachineFunction pass runs, it sees that the Carry regs are not VCC and simply marks them with a hint to later convert to VCC ,
and then continues without doing a transformation.

if (SDst) {
  if (SDst->getReg() != AMDGPU::VCC) {
    if (TargetRegisterInfo::isVirtualRegister(SDst->getReg()))
      MRI.setRegAllocationHint(SDst->getReg(), 0, AMDGPU::VCC);
    continue;
  }
 
  // All of the instructions with carry outs also have an SGPR input in
  // src2.
  if (Src2 && Src2->getReg() != AMDGPU::VCC) {
    if (TargetRegisterInfo::isVirtualRegister(Src2->getReg()))
      MRI.setRegAllocationHint(Src2->getReg(), 0, AMDGPU::VCC);
 
    continue;
  }
}

In D54882#1309708, @ronlieb wrote:

Adding the shrink pass just before Peephole SDWA does not help the lit test, it made no difference.
I think at this point i should proceed with adding the MIR test to verify when we cannot fold.

In D54882#1309639, @ronlieb wrote:

In D54882#1309605, @rampitec wrote:

In D54882#1309588, @rampitec wrote:

In D54882#1309583, @ronlieb wrote:

In D54882#1308240, @rampitec wrote:

Essentially this is a limited version of shrinking. So I have several questions:

1. Why not to run shrink pass before sdwa instead?

I tried adding Shrink pass before PeepholeSDWA and observed 88 lit test failures.
i tried moving Shrink pass before Peephole SDWA and observed 25 lit test failures

Which may be a good thing if these failures are progressions (as I suspect) and not regressions. Are they progressions?
That is the point of other comments too, this patch is limited to handle just two instructions while there is a clear possibility to do it for almost any VOP3.

I would also assume many of these failures are just commute which is attempted by shrink pass. That is normal and would only need to change the tests.

i tried an experiment of simply invoking the Shrink pass a 2nd time.

addPass(createSIShrinkInstructionsPass());
addPass(createSIShrinkInstructionsPass());

which resulted in 74 failures, and they do seem to be commute changes primarily (did not look at them all)
So then, i added a 3rd invocation and zero failures (i'm still laughing at this one).

Ping.

In D54882#1309588, @rampitec wrote:

In D54882#1309583, @ronlieb wrote:

In D54882#1308240, @rampitec wrote:

Essentially this is a limited version of shrinking. So I have several questions:

1. Why not to run shrink pass before sdwa instead?

I tried adding Shrink pass before PeepholeSDWA and observed 88 lit test failures.
i tried moving Shrink pass before Peephole SDWA and observed 25 lit test failures

Which may be a good thing if these failures are progressions (as I suspect) and not regressions. Are they progressions?
That is the point of other comments too, this patch is limited to handle just two instructions while there is a clear possibility to do it for almost any VOP3.

In D54882#1309583, @ronlieb wrote:

In D54882#1308240, @rampitec wrote:

Essentially this is a limited version of shrinking. So I have several questions:

1. Why not to run shrink pass before sdwa instead?

I tried adding Shrink pass before PeepholeSDWA and observed 88 lit test failures.
i tried moving Shrink pass before Peephole SDWA and observed 25 lit test failures

LGTM (for AMDGPU tests and core).

LGTM

Essentially this is a limited version of shrinking. So I have several questions:

LGTM

Rebased.

Added negative checks to select-undef.ll and rebased.

Converted tests to non-undef vectors, added a test with an undef vector.
This is now based on D54646 and insert_vector_elt undef, ... results just in a splat.

Added test from D48376.

In D54646#1301632, @rampitec wrote:

In D54646#1301593, @arsenm wrote:

In D54646#1301582, @rampitec wrote:

In D54646#1301556, @arsenm wrote:

This pretty much reproduces D48376, but you should copy the test I added

The test you have added there does not work anymore.
What's the new syntax for @llvm.amdgcn.image.sample.f32.v2f32.v8i32? Is it relevant to use image sample in this test at all?

I think it's because it avoids the fact that regular loads are converted to integer loads, and it was to ensure it was an FP type. You should be able to just copy any other FP returning intrinsic use from another test

Matt, I do not understand your test. I believe it does not test what's intended. For some reason your checks are:

; GCN: s_waitcnt
; GCN-NEXT: s_setpc_b64

I.e. you seem to check there is no v_cndmask_b32, though it is never checked. But it looks like you believe that an intrinsic call with undef operands will be combined into undef. This is not the case most of the times and not even a case for image sample anymore. This is almost the test for image_sample(undef) folding, not select(undef).

I understand the point of the test, just do not believe it is correct. I see two possible ways: 1) commit this as is and extend tests from D54606 to check both undef and non-undef cases 2) Commit D54606 first and then add select-undef.ll tests to this review which would use insertelement.

In D54646#1301593, @arsenm wrote:

In D54646#1301582, @rampitec wrote:

In D54646#1301556, @arsenm wrote:

This pretty much reproduces D48376, but you should copy the test I added

The test you have added there does not work anymore.
What's the new syntax for @llvm.amdgcn.image.sample.f32.v2f32.v8i32? Is it relevant to use image sample in this test at all?

I think it's because it avoids the fact that regular loads are converted to integer loads, and it was to ensure it was an FP type. You should be able to just copy any other FP returning intrinsic use from another test

In D54646#1301556, @arsenm wrote:

This pretty much reproduces D48376, but you should copy the test I added

In D54606#1301265, @rampitec wrote:

In D54606#1301250, @nhaehnle wrote:

In D54606#1301212, @rampitec wrote:

However, why does code with undef vectors look so bad? For example, in float4_inselt, the fact that the initial vector is undef should allow us to just store a splat of 1.0.

Yes, I noticed that too. That needs to be a separate optimization. As far as I understand "insert_vector_element undef, %var, %idx" should not even come to this point. It needs to be replaced by build_vector (n x %var) regardless of the thresholds and heuristics I am using, e.g. earlier (higher in the same function I think).

I disagree. When we end up using scratch memory for a vector, build_vector (n x %var) would imply n stores, while insert_vector_element undef, %var, %idx implies only 1 store, so doing the transform seems like a pretty terrible idea in general. I think exploiting undef is really specific to what you're doing here and should therefore be done here as well.

Hm... Ok, I will add it here.

In D54606#1301261, @arsenm wrote:

Another change I've wanted to look at is changing what AMDGPUPromoteAlloca tries to produce. The dynamic vector indexing is going to be worse if the waterfall loop is going to be required, but it's currently preferred if both are possible.

In D54606#1301250, @nhaehnle wrote:

In D54606#1301212, @rampitec wrote:

However, why does code with undef vectors look so bad? For example, in float4_inselt, the fact that the initial vector is undef should allow us to just store a splat of 1.0.

Yes, I noticed that too. That needs to be a separate optimization. As far as I understand "insert_vector_element undef, %var, %idx" should not even come to this point. It needs to be replaced by build_vector (n x %var) regardless of the thresholds and heuristics I am using, e.g. earlier (higher in the same function I think).

I disagree. When we end up using scratch memory for a vector, build_vector (n x %var) would imply n stores, while insert_vector_element undef, %var, %idx implies only 1 store, so doing the transform seems like a pretty terrible idea in general. I think exploiting undef is really specific to what you're doing here and should therefore be done here as well.

In D54606#1300988, @nhaehnle wrote:

Mostly looks good to me.

However, why does code with undef vectors look so bad? For example, in float4_inselt, the fact that the initial vector is undef should allow us to just store a splat of 1.0.