Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove superfluous non-operands from LDXR, STXR, and related instructions on AArch64 #4532

Closed
Tracked by #5145
derekbruening opened this issue Nov 13, 2020 · 5 comments · Fixed by #5234 or #5275
Closed
Tracked by #5145

Remove superfluous non-operands from LDXR, STXR, and related instructions on AArch64 #4532

derekbruening opened this issue Nov 13, 2020 · 5 comments · Fixed by #5234 or #5275
Labels
Component-API Component-IR OpSys-AArch64

Comments

@derekbruening
Copy link
Contributor

On AArch64 we have superfluous operands in the exclusive load and store instructions in DR's IR:

  0x0000ffff7d459b24  885f7c01   ldxr   (%x0)[4byte] $0x1f $0x1f -> %w1
  0x0000ffff7d459af0  885ffc01   ldaxr  (%x0)[4byte] $0x1f $0x1f -> %w1

  0x0000ffff7d459afc  88027c04   stxr   %w4 $0x1f -> (%x0)[4byte] %w2
  0x0000ffff7d459b28  8803fc02   stlxr  %w2 $0x1f -> (%x0)[4byte] %w3

These 0x1f integers seem to be just part of the encoding and aren't actually operands. I would expect them to not show up in the IR at all.
@derekbruening
Copy link
Contributor Author

CLREX shouldn't have this 0 either:

 +52   m4 @0x0000fffd47084ff8  d503305f   clrex  $0x0000000000000000

This was referenced Nov 19, 2020
Merged
Open
This was referenced Nov 29, 2021
Merged
Closed
joshua-warburton added a commit that referenced this issue Nov 30, 2021
@joshua-warburton
i#4532: Remove superfluous non-operands from AArch64 memory instructions
39e3b50 
Several AArch64 memory related instructions have various
bits designated as "soft bits." This means that the spec
suggests a value of 1 for them, but it is allowed for the
bits to vary from that.

Previously these bits were represented as operands, but
that caused stray values to show up in the IR. This change
introduces a way to represent soft bits in codec.txt which
tells the decoder to ignore them and the encoder to set them
to be 1.

This change also updates the encoding macros to remove the
unnecessary operands and updates the appropriate tests. The
decode tests have been modified to allow the re-encoding to
differ from the original encoding. This is because the soft
bits can be values not strictly dictated by the spec but as
we will always re-encode to spec, the re-encoding will be
different to the initial encoding.

Fixes: #4532

Change-Id: I84ed39178a0b9ee0c98ae0929dcddf9a41f9c314
@joshua-warburton joshua-warburton mentioned this issue Nov 30, 2021
Merged
@derekbruening
Copy link
Contributor Author

There are still 2 references to i#4532 in the code base: I think PR #5234 failed to remove/address those.

Also during the merge of PR #5234 the Fixes line was deleted from its final commit description: I guess it was the line in the PR itself that still managed to auto-close this?

@derekbruening
Copy link
Contributor Author

CLREX still has an immediate int operand: clrex $0x0000000000000000. The code points at this issue:

core/ir/aarch64/instr_create_api.h:/* TODO i#4532: Remove this superfluous operand. */
core/ir/aarch64/instr_create_api.h-#define INSTR_CREATE_clrex(dc) instr_create_0dst_1src(dc, OP_clrex, OPND_CREATE_INT(0))

So I think this should be re-opened.

@derekbruening derekbruening reopened this Dec 15, 2021
derekbruening added a commit that referenced this issue Dec 15, 2021
@derekbruening
i#4532 extra opnds: Remove extra opnd handling in mangling
d66e519 
Removes a no-longer-needed extra check to identify a load exclusive
pair, now that the superfluous immediate integer operands are removed.

Tested on the client.ldstex test which should fail if we incorrectly
identify a pair.

Issue: #4532
@derekbruening derekbruening mentioned this issue Dec 15, 2021
Merged
@joshua-warburton
Copy link
Collaborator

CLREX is a different case to the other instructions in that it does actually have an optional imm argument, but that argument is ignored, unlike the other superfluous arguments which were there to handle the potentially variable "sticky bits." It also defaults to a value of 15 for the imm if it is not set

This is similar to udf, which also has an ignored, optional operand.

Considering this, we should have two variants in order to comply with the spec:
11010101000000110011xxxx01011111 n 58 clrex : imm4
11010101000000110011111101011111 n 58 clrex

derekbruening added a commit that referenced this issue Dec 16, 2021
@derekbruening
i#4532 extra opnds: Remove extra opnd handling in mangling (#5253)
b56a61f 
Removes a no-longer-needed extra check to identify a load exclusive
pair, now that the superfluous immediate integer operands are removed.

Tested on the client.ldstex test which should fail if we incorrectly
identify a pair.

Issue: #4532
@derekbruening
Copy link
Contributor Author

OK, so an action item then is to remove the TODO i#4532 comment then from the INSTR_CREATE_clrex macro?

@joshua-warburton joshua-warburton mentioned this issue Jan 11, 2022
Merged
joshua-warburton added a commit that referenced this issue Jan 12, 2022
@joshua-warburton
i#4532: aarch64 decode: remove incorrect clrex todo (#5275)
8e1c9f0 
This patch removes the TODO note for clrex which stated it had a superfluous
operand. The spec states that it is a real operand but is ignored and defaults
to a value of 15. The instr_create.h macros have been updated to reflect this.

Fixes: #4532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component-API Component-IR OpSys-AArch64
Projects
None yet
Milestone
No milestone
2 participants
@derekbruening @joshua-warburton