Split the assertion into smaller chunks.

Ping

Extended only for amdgpu_cs and when the subtarget has architected SGPRs enabled.

Renamed the testcase.

The IsPredicable field will always be turned to 1 for Pred_Copy to indicate that it is the Predicated version of the copy.

In D143753#4131294, @qcolombet wrote:

Hey,

What's the long term plan for this helper?

I am not mad at the refactoring itself, but I'm wondering what kind of criteria we should use to decide when we add such helper.

The real motivation for these buildCopy helper functions is to minimize the additional code changes required while introducing getCopyOpcode() as part of D143754.
One could probably say that "still you could have avoided the buildCopy functions, and instead replace TargetOpcode::COPY with getCopyOpcode()".
I think it would be ok to refactor the code if found an opportunity while working around that code.

For instance, should we do the same thing for PHI, INSERT_SUBREG, etc. and why or why not.
Also, should these helper be virtual or not?

As I said earlier, COPY wasn't a random pick, but rather a code refactor for another patch.

I guess where I am going is what do you intend to simplify/achieve with them?

I felt it was better with buildCopy rather than the direct replacement of TargetOpcode::COPY with getCopyOpcode() in their original BuildMI instances. You tend to agree with that towards the end - "(I actually like the patch)".

The bottom line is without a proper rationale on why we need this helper, I feel that it gets difficult to know which method should be used to construct a copy.
Right now we have BuildMI (and the underlying MachineInstrBuilder object that can be called directly), MachineFunction::CreateMachineInstr, MachineIRBuilder::buildInstr (BTW MachineIRBuilder has its own MachineIRBuilder::buildCopy) and we add a new method here.

I agree. All my buildCopy instances currently return MachineInstr*. I wasn't sure they should return MachineInstrBuilder or any other form as you mentioned. It can be worked out if anyone has a better suggestion.
And about MachineIRBuilder::buildCopy, I feel it is better to retain this very specific instance considering the actual function being called ( the generic MachineIRBuilder::buildInstr) and the operands used which are different from the instances I added.

I believe we need to disambiguate when this one should be used and why.

Admittedly I'm playing the devil's advocate here (I actually like the patch) but I think it is important to flush out the goals and intended usages to be able to better guide future users of this (and the other) API(s).

Cheers,
-Quentin

RFC posted https://discourse.llvm.org/t/rfc-introduce-generic-predicated-copy-opcode/68494

Added an assertion as per review comment.

Adjusted NumRequiredSystemSGPRs before force initializing the 16 input SGPRs.

D143757 in the review chain enables predicated copy for AMDGPU target.
The application of pred-copy is here: D143762.
Testcase llvm/test/CodeGen/AMDGPU/whole-wave-register-copy.ll added along with D124196 demonstrates the need for pred-copy.

Rebased after whole-wave copy implementation.

D138656 handled it better and hence abanding it.

In D124196#4007017, @jdoerfert wrote:

This patch causes OpenMC offloaded via OpenMP on AMDGPUs to crash at runtime. It looks like some corruption in the memory address.
You can find build instructions here: https://github.com/jtramm/openmc_offloading_builder

The commit before this one works fine though, assuming you cherry picked https://reviews.llvm.org/rGee1d000d43321590771a2f047c8c55d07d09ad28 first as it landed after.
I assume other codes will be impacted too.

@jtramm @ronlieb @jhuber6 FYI

rebase

I will wait for a couple of days before I upstream it to see if there is any comment from others.

Fixed the comment appropriately.

code rebase

code rebase

code rebase

code rebase

code rebase

Code rebase

Rebased

Addressed the review comment.

Ping

Ping. Much appreciate if this can be reviewed.

Ping

Posted D138656 to pass the virtual register as an additional argument to the spiller interface. This review can be abandoned once D138656 gets approved.

This is a better and a stable approach than D138515.
The use case of the extra argument can be found in D124196. See the implementations SIInstrInfo::storeRegToStackSlot and SIInstrInfo::loadRegFromStackSlot.

Implemented the WWM spill during RegAllocFast using the additional argument to the spiller interface introduced with patch D138656.

The idea of mapping a physical register into the current virtual register in RegAllocFast would be likely a risky choice with a global variable. The same was true for the initial patch I posted D134951 which does the same thing specifically around the spill and reload functions via. the delegate.
The effort was to identify the physical register allocated for some special machine operands (whole-wave vector registers for AMDGPU), especially inside storeRegToStackSlot & loadRegFromStackSlot spill interfaces called from fast regalloc that directly spills the physRegs.
The better approach would be by passing an additional parameter (the virtual register) to the spiller interface. The interface is common to Greedy, PEI, and to some other places that have no use for this additional parameter. Still, I think it is the right way to handle the problem.

See the Usecase of PhysToCurrentVirtReg in D124196 (look for getPhysToCurrentVirtReg)

PhysToCurrentVirtReg should also be updated while reloading a physical register from the spill location. It happens when a RegMask (for instance, a function call) clobbers a register that is used in an instruction later in the BB. Also, at a BB entry, the LiveIn registers are reloaded. The reload point begins a new live range and it should be properly reflected in the mapping.

Rebase + Incorporated changes after D138515 to move the handling of physReg to current VirtReg mapping entirely into the generic design.

This should replace the patch D134951 which used the delegate method for a specific purpose.
This is a more generic approach for targets to help identify the exact live range (virtual register) a physical register was currently assigned for.
While the allocation is in progress, this would serve the purpose of mapping a physical register to its virtual register.

Ping

Rebase + Suggestions incorporated.

Removed the IsIgnored flag and special-handled FP spill & restore cases.

For the downstream case, can we just erase the entry in the map after emitting the spill?

Can do that.

Renamed IsPEI to IsPrologEpilog & removed the unwanted comment.

Fixed a typo in the IR test file name.