[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr #75693

cygao90 · 2023-12-16T08:41:26Z

This fixes #73109.
In instruction addl %eax %rax, because there is a missing comma in the middle of two registers, the asm parser will treat it as a binary expression.

%rax  %  rax --> register mod identifier

However, In MCExpr::evaluateAsRelocatableImpl, it only checks the left side of the expression. This patch ensures the right side will also be checked.

github-actions · 2023-12-16T08:41:41Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2023-12-16T08:42:06Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-mc

Author: Chenyang Gao (cygao90)

Changes

This fixes #73109.
In instruction addl %eax %rax, because there is a missing comma in the middle of two registers, the asm parser will treat it as a binary expression.

%rax  %  rax --&gt; register mod identifier

However, In MCExpr::evaluateAsRelocatableImpl, it only checks the left side of the expression. This patch ensures the right side will also be checked.

Full diff: https://github.com/llvm/llvm-project/pull/75693.diff

1 Files Affected:

(modified) llvm/lib/MC/MCExpr.cpp (+11-10)

diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp
index 73e6569f96e463..4bed3d2d7a1c94 100644
--- a/llvm/lib/MC/MCExpr.cpp
+++ b/llvm/lib/MC/MCExpr.cpp
@@ -942,16 +942,17 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
                                                   Addrs, InSet)) {
       // Check if both are Target Expressions, see if we can compare them.
       if (const MCTargetExpr *L = dyn_cast<MCTargetExpr>(ABE->getLHS())) {
-        const MCTargetExpr *R = cast<MCTargetExpr>(ABE->getRHS());
-        switch (ABE->getOpcode()) {
-        case MCBinaryExpr::EQ:
-          Res = MCValue::get(L->isEqualTo(R) ? -1 : 0);
-          return true;
-        case MCBinaryExpr::NE:
-          Res = MCValue::get(L->isEqualTo(R) ? 0 : -1);
-          return true;
-        default:
-          break;
+        if (const MCTargetExpr *R = dyn_cast<MCTargetExpr>(ABE->getRHS())) {
+          switch (ABE->getOpcode()) {
+          case MCBinaryExpr::EQ:
+            Res = MCValue::get(L->isEqualTo(R) ? -1 : 0);
+            return true;
+          case MCBinaryExpr::NE:
+            Res = MCValue::get(L->isEqualTo(R) ? 0 : -1);
+            return true;
+          default:
+            break;
+          }
         }
       }
       return false;

phoebewang · 2023-12-16T10:09:52Z

Thanks for the fix! I think it would be a typo when added the code in 7fd992a
But I'd like to invite @rnk to double confirm it.

phoebewang

I found a problem when built it locally:

$ echo 'setb %al %rbx' | llvm-mc --show-encoding
        .text
        setb    (%al)%rbx                       # encoding: [0x0f,0x92,0x04,0x25,A,A,A,A]
                                        #   fixup A - offset: 4, value: (%al)%rbx, kind: reloc_signed_4byte

In this case, %rbx is taken as a relocation, which is incorrect.
I think the cast is intended here. What we should do is to make it user friendly by emitting an error with message. But it seems not easy to do here.

cygao90 · 2023-12-16T15:59:59Z

@phoebewang Thank you for your review. I tested on gcc. gcc will also accept such instruction:
https://godbolt.org/z/er5zEhThY
but assembler will reject it:
https://godbolt.org/z/4EnPq7rjq
Perhaps we should leave it to llvm assembler

echo 'setb %al %rbx' | ./llvm-mc --filetype=obj -o test
<stdin>:1:1: error: expected relocatable expression
setb %al %rbx
^

MaskRay · 2023-12-16T17:54:28Z

I fixed MCBinaryExpr::NE and extended llvm/test/MC/X86/register-assignment.s, but did not think much about non-MCTargetExpr RHS.

I think this patch is correct but we need a test to llvm/test/MC/X86/register-assignment.s. It can be .if var_cata == 1 ...

A better subject line is probably [MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr

phoebewang

Makes sense to me, please address @MaskRay 's comments.

cygao90 · 2023-12-17T10:23:05Z

I don't know why this check failed...

phoebewang · 2023-12-17T10:49:03Z

Check fail occurs sometimes for other reasons like infrastructure issue or problem in recent merged patches. You just need to take a look at the failed tests and ignore it if they are unrelated to your change.

cygao90 · 2023-12-17T11:20:23Z

Failed Tests (46):
--
  | MLIR :: python/dialects/affine.py
  | MLIR :: python/dialects/amdgpu.py
  | MLIR :: python/dialects/arith_dialect.py
  | MLIR :: python/dialects/builtin.py
  | MLIR :: python/dialects/cf.py
  | MLIR :: python/dialects/complex_dialect.py
  | MLIR :: python/dialects/func.py
  | MLIR :: python/dialects/linalg/opdsl/arguments.py
  | MLIR :: python/dialects/linalg/opdsl/assignments.py
  | MLIR :: python/dialects/linalg/opdsl/doctests.py
  | MLIR :: python/dialects/linalg/opdsl/emit_convolution.py
  | MLIR :: python/dialects/linalg/opdsl/emit_fill.py
  | MLIR :: python/dialects/linalg/opdsl/emit_matmul.py
  | MLIR :: python/dialects/linalg/opdsl/emit_misc.py
  | MLIR :: python/dialects/linalg/opdsl/emit_pooling.py
  | MLIR :: python/dialects/linalg/opdsl/metadata.py
  | MLIR :: python/dialects/linalg/opdsl/shape_maps_iteration.py
  | MLIR :: python/dialects/linalg/opdsl/test_core_named_ops.py
  | MLIR :: python/dialects/linalg/ops.py
  | MLIR :: python/dialects/math_dialect.py
  | MLIR :: python/dialects/memref.py
  | MLIR :: python/dialects/ml_program.py
  | MLIR :: python/dialects/nvgpu.py
  | MLIR :: python/dialects/nvvm.py
  | MLIR :: python/dialects/ods_helpers.py
  | MLIR :: python/dialects/pdl_ops.py
  | MLIR :: python/dialects/python_test.py
  | MLIR :: python/dialects/quant.py
  | MLIR :: python/dialects/scf.py
  | MLIR :: python/dialects/shape.py
  | MLIR :: python/dialects/tensor.py
  | MLIR :: python/dialects/transform.py
  | MLIR :: python/dialects/transform_bufferization_ext.py
  | MLIR :: python/dialects/transform_structured_ext.py
  | MLIR :: python/dialects/vector.py
  | MLIR :: python/ir/array_attributes.py
  | MLIR :: python/ir/attributes.py
  | MLIR :: python/ir/blocks.py
  | MLIR :: python/ir/builtin_types.py
  | MLIR :: python/ir/diagnostic_handler.py
  | MLIR :: python/ir/dialects.py
  | MLIR :: python/ir/location.py
  | MLIR :: python/ir/operation.py
  | MLIR :: python/ir/symbol_table.py
  | MLIR :: python/ir/value.py
  | MLIR :: python/pass_manager.py

Seems unrelated.

KanRobert · 2023-12-18T03:06:28Z

llvm/test/MC/X86/register-assignment-error.s

@@ -0,0 +1,8 @@
+// RUN: not llvm-mc -triple x86_64-unknown-unknown %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR


Minor suggestion

You don't need -unknown-unknown, x86_64 is enough. Ony one check prefix is used, --check-prefix=CHECK-ERROR is redundant.

…s MCTargetExpr

cygao90 · 2023-12-20T08:03:33Z

Can someone with write access merge this PR if no further change is required? Thanks.

…s and improve the test To actually address my review comment in #75693

MaskRay · 2023-12-29T03:46:18Z

Can someone with write access merge this PR if no further change is required? Thanks.

We should add a test .ifdef 1 == var_xdata as well and ensure there is an EOL at the end of the file. I merged the new test file into register-assignment.s in a22c8ef.

llvmbot added the mc Machine (object) code label Dec 16, 2023

phoebewang requested a review from rnk December 16, 2023 10:07

phoebewang requested changes Dec 16, 2023

View reviewed changes

phoebewang approved these changes Dec 17, 2023

View reviewed changes

cygao90 changed the title ~~[MC][x86] Fix missing check in MC binary expression~~ [MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr Dec 17, 2023

cygao90 force-pushed the issue#73109 branch from cd8e4f7 to 5c05add Compare December 17, 2023 08:24

llvmbot added the backend:X86 label Dec 17, 2023

KanRobert reviewed Dec 18, 2023

View reviewed changes

[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr i…

fbe0cc2

…s MCTargetExpr

cygao90 force-pushed the issue#73109 branch from 5c05add to fbe0cc2 Compare December 18, 2023 03:35

KanRobert approved these changes Dec 18, 2023

View reviewed changes

phoebewang merged commit f72b654 into llvm:main Dec 20, 2023

MaskRay added a commit that referenced this pull request Dec 29, 2023

[MC,test] Merge register-assignment-error.s into register-assignment.…

a22c8ef

…s and improve the test To actually address my review comment in #75693

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr #75693

[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr #75693

Uh oh!

cygao90 commented Dec 16, 2023

Uh oh!

github-actions bot commented Dec 16, 2023

Uh oh!

llvmbot commented Dec 16, 2023 •

edited

Loading

Uh oh!

phoebewang commented Dec 16, 2023

Uh oh!

phoebewang left a comment

Uh oh!

cygao90 commented Dec 16, 2023

Uh oh!

MaskRay commented Dec 16, 2023 •

edited

Loading

Uh oh!

phoebewang left a comment

Uh oh!

cygao90 commented Dec 17, 2023

Uh oh!

phoebewang commented Dec 17, 2023

Uh oh!

cygao90 commented Dec 17, 2023 •

edited

Loading

Uh oh!

KanRobert Dec 18, 2023

Uh oh!

cygao90 commented Dec 20, 2023

Uh oh!

MaskRay commented Dec 29, 2023

Uh oh!

Uh oh!

		@@ -0,0 +1,8 @@
		// RUN: not llvm-mc -triple x86_64-unknown-unknown %s -o /dev/null 2>&1 \| FileCheck %s --check-prefix=CHECK-ERROR

[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr #75693

[MC][x86] Allow non-MCTargetExpr RHS when the LHS of a MCBinaryExpr is MCTargetExpr #75693

Uh oh!

Conversation

cygao90 commented Dec 16, 2023

Uh oh!

github-actions bot commented Dec 16, 2023

Uh oh!

llvmbot commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoebewang commented Dec 16, 2023

Uh oh!

phoebewang left a comment

Choose a reason for hiding this comment

Uh oh!

cygao90 commented Dec 16, 2023

Uh oh!

MaskRay commented Dec 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoebewang left a comment

Choose a reason for hiding this comment

Uh oh!

cygao90 commented Dec 17, 2023

Uh oh!

phoebewang commented Dec 17, 2023

Uh oh!

cygao90 commented Dec 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KanRobert Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

cygao90 commented Dec 20, 2023

Uh oh!

MaskRay commented Dec 29, 2023

Uh oh!

Uh oh!

llvmbot commented Dec 16, 2023 •

edited

Loading

MaskRay commented Dec 16, 2023 •

edited

Loading

cygao90 commented Dec 17, 2023 •

edited

Loading