Page MenuHomePhabricator

[Targets] Implement getConstraintRegister for ARM and AArch64
ClosedPublic

Authored by miyuki on Apr 23 2018, 7:47 AM.

Details

Summary

The getConstraintRegister method is used by semantic checking of
inline assembly statements in order to diagnose conflicts between
clobber list and input/output lists. Currently ARM and AArch64 don't
override getConstraintRegister, so conflicts between registers
assigned to variables in asm labels and clobber lists are not
diagnosed. Such conflicts can cause assertion failures in the back end
and even miscompilations.

This patch implements getConstraintRegister for ARM and AArch64
targets. Since these targets don't have single-register constraints,
the implementation is trivial and just returns the register specified
in an asm label (if any).

Diff Detail

Repository
rC Clang

Event Timeline

miyuki created this revision.Apr 23 2018, 7:47 AM
thopre added inline comments.Apr 23 2018, 9:27 AM
lib/Basic/Targets/AArch64.h
85–89

From what I understood of the original patch, getConstraintRegister is a sort of a more comprehensive version of convertRegister. On ARM convertRegister handles U and p constraint specially, should this do the same?

miyuki added inline comments.Apr 24 2018, 2:21 AM
lib/Basic/Targets/AArch64.h
85–89

Did you mean convertConstraint? As I understand, this function does canonicalization of constraints. If a constraint happens to be single-register, it is converted into {register} form (but this does not happen on ARM). On the other hand, getConstraintRegister takes a constraint and an asm label and converts them into a register (and in our case the register always comes from the asm label, never from the constraint), so I don't think we need to handle U and p specially.

miyuki updated this revision to Diff 143732.Apr 24 2018, 6:50 AM

Added a comment for getConstraintRegister

thopre added inline comments.Apr 24 2018, 6:53 AM
lib/Basic/Targets/AArch64.h
85–89

Forgot to send this:

Fair enough. Since you seems to have the expected behavior of this function well understood, would you mind adding a comment here and in clang/Basic/TargetInfo.h?

Luckily we had a similar chat offline. Thanks for the change

thopre accepted this revision.Apr 24 2018, 7:06 AM

As I said, LGTM. Please wait a few days (say next Monday) in case anyone else has any objection.

This revision is now accepted and ready to land.Apr 24 2018, 7:06 AM
This revision was automatically updated to reflect the committed changes.