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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
