correcting X86OutgoingValueHandler typo (NFC)

[CSears]

Summary

The X86OutgoingValueHandler declaration has a typo. It subclasses IncomingValueHandler rather than OutgoingValueHandler. This isn't actually a 'bug' because both IncomingValueHandler and OutgoingValueHandler have the same 'shape'. It's just a typo. This typo is only in the X86 backend.

[craig.topper]

Doesn't the bug cause isIncomingArgumentHandler() to return the wrong value? Does that not create an issue for CallLowering::handleAssignments?

[CSears]

I don't think it does but it could because IncomingValueHandler passes true
to the ValueHandler constructor and OutgoingValueHandler passes false. That
boolean then gets stored in IsIncoming. However, the only code which uses
IsIncoming is AMDGPU.

[arsenm]

In D92631#2433822, @CSears wrote:

I don't think it does but it could because IncomingValueHandler passes true
to the ValueHandler constructor and OutgoingValueHandler passes false. That
boolean then gets stored in IsIncoming. However, the only code which uses
IsIncoming is AMDGPU.

The generic code also checks it

[CSears]

Yes. CallLowering::handleAssignments() calls the isIncomingArgumentHandler() getter.

Arm and AArch64 subclass from CallLowering::OutgoingValueHandler and CallLowering::IncomingValueHandler. No problem there. X86 has the typo.

PPC and RISCV don't create value handlers. No problem there.
AMDGPU subclasses from its own ValueHandler and sets isComing accordingly. No problem there.

Mips creates its own MipsHandler (which doesn't subclass from ValueHandler) and subclasses from that. It doesn't set isIncoming at all (defaults to false) and doesn't use it. It also doesn't call handleAssignments() and instead has its own handle(). There doesn't seem to be a problem.

[craig.topper]

In D92631#2434315, @CSears wrote:

Yes. CallLowering::handleAssignments() calls the isIncomingArgumentHandler() getter.

Arm and AArch64 subclass from CallLowering::OutgoingValueHandler and CallLowering::IncomingValueHandler. No problem there. X86 has the typo.

PPC and RISCV don't create value handlers. No problem there.
AMDGPU subclasses from its own ValueHandler and sets isComing accordingly. No problem there.

Mips creates its own MipsHandler (which doesn't subclass from ValueHandler) and subclasses from that. It doesn't set isIncoming at all (defaults to false) and doesn't use it. It also doesn't call handleAssignments() and instead has its own handle(). There doesn't seem to be a problem.

X86 has a typo and calls handleAssignments() which uses isIncomingArgumentHandler(). So why is this patch NFC? Is X86 not using a code path in handleAssignments() cares about isIncomingArgumentHandler()?

[CSears]

The NFC was my estimation given that I thought it was a typo. I didn't see that handleAssignments() used the flag. (I searched for the flag rather than for the getter.) Given that handleAssignments() uses it, it isn't an NFC typo. It is a bug.

[CSears]

Matt, I do not have commit privileges. If you can commit on my behalf (I think that's how it works) I'd appreciate it.

llvm/lib/Target/X86/X86CallLowering.cpp

 
 namespace {
 
-struct X86OutgoingValueHandler : public CallLowering::IncomingValueHandler {
+struct X86OutgoingValueHandler : public CallLowering::OutgoingValueHandler {
   X86OutgoingValueHandler(MachineIRBuilder &MIRBuilder,
                           MachineRegisterInfo &MRI, MachineInstrBuilder &MIB,
                           CCAssignFn *AssignFn)
-      : IncomingValueHandler(MIRBuilder, MRI, AssignFn), MIB(MIB),
+      : OutgoingValueHandler(MIRBuilder, MRI, AssignFn), MIB(MIB),
         DL(MIRBuilder.getMF().getDataLayout()),
         STI(MIRBuilder.getMF().getSubtarget<X86Subtarget>()) {}