This is an archive of the discontinued LLVM Phabricator instance.

[THUMB2] add .w suffixes for ldr/str (immediate) T4
ClosedPublic

Authored by nickdesaulniers on Feb 12 2021, 1:46 PM.

Details

Summary

The Linux kernel when built with CONFIG_THUMB2_KERNEL makes use of these
instructions with immediate operands and wide encodings.

These are the T4 variants of the follow sections from the Arm ARM.
F5.1.72 LDR (immediate)
F5.1.229 STR (immediate)

I wasn't able to represent these simple aliases using t2InstAlias due to
the Constraints on the non-suffixed existing instructions, which results
in some manual parsing logic needing to be added.

F1.2 Standard assembler syntax fields
describes the use of the .w (wide) vs .n (narrow) encoding suffix.

Link: https://bugs.llvm.org/show_bug.cgi?id=49118
Link: https://github.com/ClangBuiltLinux/linux/issues/1296
Reported-by: Stefan Agner <stefan@agner.ch>
Reported-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Feb 12 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 1:46 PM

There's some TODO's I plan to clean up in the test, but looking for feedback before I pursue further additions.

TLDR: I tried to track down the meaning of .w for the assembler and the encodings. They seem to be slightly different but I think the assembler side is clear and that's what we need here.

The explanation of .w in the armarm:

A8.2 Standard assembler syntax fields

Specifies optional assembler qualifiers on the instruction.

The following qualifiers are defined:
.N Meaning narrow, specifies that the assembler must select a 16-bit encoding for the
instruction. If this is not possible, an assembler error is produced.

.W Meaning wide, specifies that the assembler must select a 32-bit encoding for the
instruction. If this is not possible, an assembler error is produced.

If neither .W nor .N is specified, the assembler can select either 16-bit or 32-bit encodings. If both
are available, it must select a 16-bit encoding. In a few cases, more than one encoding of the same
length can be available for an instruction. The rules for selecting between such encodings are
instruction-specific and are part of the instruction description

Which explains the assembler's point of view. Less clear, is what a .W means in an instruction description.

A8.1.3 Instruction encodings

For example, the assembler
syntax documented for the 32-bit Thumb AND (register) encoding includes the .W qualifier to ensure that the
32-bit encoding is selected even for the small proportion of operand combinations for which the 16-bit
encoding is also available.

Which is not a great example because when I tested this with gcc, it would prefer 16 bit encodings in most situations
that I could come up with. However, AND has conditions for being in or out of an IT block so that might be higher priority than any .w encoding.

For LDR there's no IT block stuff to consider, but gcc still prefers the smaller encoding if possible.

.syntax unified

// This could fit into 16 bit T1, but should prefer T3 with the .w?
// Uses T1
// (which seems to contradict the AND example but hey)
ldr r3, [r1, #124]
// Will be T3 due to us adding .w
ldr.w r3, [r1, #124]
// This can only fit into T3 because of the 12 bit immediate
ldr r3, [r1, #0x1ff]
// T3 because it has the .W suffix in the description, could fit into T4 as well
ldr.w r3, [r1, #255]
// Uses T4 due to post increment
ldr.w r3, [r1], #255

So my best guess is that I am misunderstanding the AND example and that .W marks the preferred 32 bit thumb encoding.

The assembler will:

  • try to use 16 bits
  • try to use 32 bits
  • if there are multiple 32 bit encodings that could apply, use the one with .W

Note that there are some instructions like ADR where T2 and T3 have .W. In this case (and I assume other cases) the two clearly wouldn't overlap so it's still decidable which one should be used.
Also, some instructions have only one 32 bit encoding, which has .W. I expect this just makes the logic easier if you can say "every instruction that has 32 bit encodings has a preferred one".

This is all to support reassembly to the same bit patterns. If we assemble "ldr.w r3, [r1, #255]" and get T3, then disassemble, we get "ldr.w r3, [r1, #255]". Reassembling that we want to know we'll get T3 not T4.

Caveat: I can't cite anywhere in the armarm that says ".W means preferred 32 bit". That's just the conclusion I'm drawing from what I've found.

I just realised that you're only adding the ones that would encode to T4, so the answer to some of my comments will be no, T3 isn't covered. Correct?

I'm not sure how much complexity adding T3 would be. We're ok to encode any pre increment, non writeback into T3 so it could be another alias, with a different immediate check.

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

Do you mean "$Rn != $Rn_wb". I see what you mean but out of the context of the armarm, != makes more immediate sense.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
7672

You'll add a check here for PRE too, or is that not needed for some reason?

This would use the imm12 range from the T3 encoding I think.

llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
82

Add pre-increment, no writeback here.

92

You should check pre-increment without writeback, that would use the T3 encoding and have a greater immediate range.

106

For varying the registers you could cut it down to:

  • one gpr
  • pc (if allowed)
  • sp (if allowed)

There's not much utility to the rest.

127

I'd just pick two here. That shows we pass them through correctly, we can assume the rest are ok.

297

pre-increment without writeback

A8.1.3 Instruction encodings

For example, the assembler
syntax documented for the 32-bit Thumb AND (register) encoding includes the .W qualifier to ensure that the
32-bit encoding is selected even for the small proportion of operand combinations for which the 16-bit
encoding is also available.

Which is not a great example because when I tested this with gcc, it would prefer 16 bit encodings in most situations
that I could come up with. However, AND has conditions for being in or out of an IT block so that might be higher priority than any .w encoding.

Note that "the assembler" in the quoted text is not GAS specifically. It is the rule that any "conforming" arm assembler must follow.

However, avoiding this particular rule won't generate execution errors, just pick the wrong encoding for the exact same instruction, so if GAS is picking the wrong encoding, it probably decided it was worth it.

This doesn't mean LLVM should do the same. We strongly prefer to follow the rules of the architecture manuals, when available.

nickdesaulniers marked 2 inline comments as done.
  • add more to comment in .td.
  • add parser check for str to pc.
  • fix case lint.
  • move TODOs for cases where the non-.w suffix already differs from GAS.
  • add comment that T4 PRE is with writeback.
  • significantly reduce number of test cases.
llvm/lib/Target/ARM/ARMInstrThumb2.td
1568

No; that's what's above for t2LDR_POST (I guess it should be "$addr.base = $Rn_wb" for PRE in my added comment). I tried wrapping these in let blocks while using t2InstAlias to describe the "Constraint"s but it seems I may not be the first to have hit that: https://reviews.llvm.org/D68916#1741416.

I've updated the comment.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
7672

I'm not really sure if operands have additional validation, but I suspect that the use of t2addrmode_imm8_pre for PRE is validated elsewhere; and perhaps t2am_imm8_offset used for POST is not. If I add checks for pre here and feed in bad input, it wont matter, because we never get this far. I suspect the operand validation is occurring before instruction matching.

For example, consider:

ldr.w r3, [r1, #4095]!

(so T4 pre increment w/ writeback, only accepts immediates in the range [-255,255]). With this patch applied:

error: invalid instruction, any one of the following would fix this:
ldr.w r3, [r1, #4095]!
^
foo.s:17:11: note: invalid operand for instruction
ldr.w r3, [r1, #4095]!
          ^
foo.s:17:22: note: too many operands for instruction
ldr.w r3, [r1, #4095]!
                     ^

which looks to me like the instruction is not getting matched, BEFORE any of this added parsing logic. Perhaps the error text for t2addrmode_imm8_pre could be improved, somehow?

llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
82
nickdesaulniers edited the summary of this revision. (Show Details)Feb 19 2021, 4:14 PM
nickdesaulniers retitled this revision from [THUMB2] add .w suffixes for ldr/str w/ immediates to [THUMB2] add .w suffixes for ldr/str (immediate) T4.
nickdesaulniers edited the summary of this revision. (Show Details)
DavidSpickett added inline comments.Feb 22 2021, 4:34 AM
llvm/lib/Target/ARM/ARMInstrThumb2.td
1568

Makes sense now.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
7672

I'm sure it could be improved but out of scope here. My concern was that we would leave a gap where valid input would be encoded to T4 but not T3. However, clang already accepts the T3 input so it's not an issue.

ldr.w r3, [r1, #4095]
$ ./bin/clang --target=arm-none-eabi -march=armv7-a /tmp/test.s -c -o /tmp/test.o -Wa,-mthumb
llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
82
92
123

Do you have a plan for how to implement these? I assume it's complicated in the same way the alias constraints are.

297

Addressed above.

DavidSpickett added inline comments.Feb 22 2021, 4:36 AM
llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
123

Should be possible to warn by "return Warning" from ARMAsmParser::validateInstruction but I haven't tried it myself.

nickdesaulniers marked 5 inline comments as done.Feb 22 2021, 9:44 AM
nickdesaulniers added inline comments.
llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
123

I don't plan to implement those in this commit. The underlying aliased-to instruction doesn't warn.

Specifically, the warning GAS produces is This instruction may be unpredictable if executed on M-profile cores with interrupts enabled.

I'm not sure we have such a warning.

DavidSpickett accepted this revision.Feb 23 2021, 3:28 AM
DavidSpickett added inline comments.
llvm/test/MC/ARM/thumb2-ldr.w-str.w.s
123

Ah I thought it was the usual sp/pc is always unpredictable stuff. Fine to leave it out here then.

This revision is now accepted and ready to land.Feb 23 2021, 3:28 AM