[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

Wed Jan 12 11:02:49 PST 2022

jonpa added a comment.

Sorry for the delay.

> This change seems like it's done in the wrong layer -- why do we add a special-case in Clang to emit "={r11},{r11}", instead of fixing LLVM to not break when given the previously-output "={r11},0"?

We were starting out with a test case that was the result of macro-intensive code (Linux kernel). It had a tied physreg def/use, with an additional use of the same physreg, which did not compile with clang. There were two problems: TwoAddress could not handle this case and therefore asserted, and also the extra COPY in the output of the same physreg that would also have to be dealt with.

Instead of trying to improve TwoAddress to recognize and handle a special case like this while also making sure no redundant COPYs would be produced, we found it more reasonable to handle this in the front end. The reasoning was that "= {r7},0" should be completely equivalent with "= {r7},{r7}", and they should be treated the same by the compiler. A tied physreg did not seem useful in the first place, so we decided to always emit the second form from the front end.

This is pretty much what the original commit message said: "Background: Macro intensive user code may contain inline assembly statements with multiple operands constrained to the same physreg. Such a case (with the operand constraints "+r" : "r") currently triggers the TwoAddressInstructionPass assertion against any extra use of a tied register. Furthermore, TwoAddress will insert a COPY to that physreg even though isel has already done so (for the non-tied use), which may lead to a second redundant instruction currently. A simple fix for this is to not emit tied physreg uses in the first place for the "+r" constraint, which is what this patch does."

> Furthermore, since earlyclobber was exempted from this change, doesn't the same TwoAddressInstructionPass bug still affect earlyclobber uses?

I was never under the impression that this was a bug in TwoAddress, but rather an unhandled ("odd") case of phys-regs. ISel produced the first COPY, and TwoAddress the second one. How should that be cleaned up? Seemed best to avoid adding unnecessarily to TwoAddress if possible to avoid it...

Sorry, we don't have early clobber operands on SystemZ, so I haven't really thought much about that. We decided we had to exclude them from this patch per the reasoning of https://reviews.llvm.org/D87279#2334519.

So I would guess it might still be a problem to compile an early-clobber tied def/use of a physreg with an extra use (or several) of the same physreg. If that case is something you intend to handle, I would agree that perhaps then the front end change of this patch would not be needed anymore as it is the alternative approach.

> @nathanchance reports in https://github.com/ClangBuiltLinux/linux/issues/1512 that... now seems to be crashing as a result of this change.

I tested x.c, and it "works" on SystemZ, while it did not pass ISel on X86, from what I understand. Given the above reasoning of the equivalency between the two expressions (use either tied or explicit of the same phys-reg), I would also wonder if X86 is "choking on valid IR"...

As another matter, the SystemZ backend actually can't handle x.c, as it triggers `SpillGPRs.LowGPR != SpillGPRs.HighGPR && "Should be saving %r15 and something else". This is however a problem in the prologe/epilog emission if anything... I guess the case of just clobbering SP in a function has not been seen before.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87279/new/

https://reviews.llvm.org/D87279