Page MenuHomePhabricator

[ARM] Accept ldrb.w mnemonic for certain addressing modes (PR43382)
Needs ReviewPublic

Authored by chill on Oct 12 2019, 11:18 AM.

Details

Reviewers
efriedma
grosbach
Summary

Encoding T3 of ldrb in A7.7.46 (Armv7-M ARM Revidion E.d) offset allows the ".w" mnemonic suffix, even
though the preferred disassembly is without the suffix.
We did not accept the suffix; this patch fixes it by adding a few instruction aliases.

Is there a less hackish way of doing it ?

Diff Detail

Event Timeline

chill created this revision.Oct 12 2019, 11:18 AM

Adding aliases seems like the right approach; we have aliases for a bunch of other instructions.

I'm not sure why you need the AsmMatchConverter, though; needs a comment to explain.

llvm/lib/Target/ARM/ARMInstrThumb2.td
1536

Why is Rt listed twice here?

chill added a comment.Oct 14 2019, 3:54 PM

Adding aliases seems like the right approach; we have aliases for a bunch of other instructions.

I'm not sure why you need the AsmMatchConverter, though; needs a comment to explain.

Why is Rt listed twice here?

For both aliases we need something to match the writeback out operand of t2LDRB_PRE and t2LDRB_POST.
For t2LDR_PRE the writeback is well hidden in $addr operand and something like $addr.base does not seem to work.
For t2LDR_POST the $Rn operand is not a GPR, it's a memory operand and likewise does not match.

$Rt is just a placeholder in both cases.

And that's why we need the AsmMatchConverters - to replace *that* Rt with the correct writeback register from the memory operands.

Oh, that makes sense. But needs a comment on the pattern, and in cvtThumb2LDRB_PRE/POST.

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

Do other PRE/POST load/store instructions have the same issue?

sdesmalen added inline comments.Oct 15 2019, 1:36 AM
llvm/lib/Target/ARM/ARMInstrThumb2.td
1404

Does LDRH (and maybe other instructions) not suffer the same issue? If so, should these InstAliases be moved into T2I_ld?

chill marked an inline comment as done.Oct 17 2019, 10:49 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMInstrThumb2.td
1404

Yes, other instructions have the same issue. I plan to fix (some of) them in separate patches, once the approach with AsmMatchConverter is validated as good enough. Then I'll look for opportunities to refactor.

chill updated this revision to Diff 228602.Nov 10 2019, 4:02 AM

Added some comments.

chill marked 2 inline comments as done.Nov 10 2019, 4:04 AM

Do you have a reply to the following?

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

Do you have a reply to the following?

It looks like the normal instruction patterns resolve the issue using "Constraints", to specify "Rn_wb". I guess that isn't possible for aliases, though, at least at the moment?

From my discussion with @chill today, that seems to be the issue indeed. TableGen doesn't use the $addr.base = $Rn_wb constraint from the instruction to materialize the second output register for the InstAliases. This is probably best fixed in TableGen itself, but having seen some of that code I can appreciate there is a tradeoff here whether or not to go the AsmMatchConverter route.