X86 Asm can't work properly with symbolic Scale
AbandonedPublic

Authored by avt77 on Jul 19 2017, 7:22 AM.

Details

Summary

The current Asm does not support symbolic Scale at all. It's only possible to use immediate as Scale. This patch is the first one to close PR33848

Diff Detail

avt77 created this revision.Jul 19 2017, 7:22 AM
avt77 updated this revision to Diff 107506.Jul 20 2017, 7:45 AM

I updated 2 tests in trunk to demonstarte the difference in code generation after this patch aplying. Now we can clearly see the issues and how they could be resolved with help of this patch.

mcrosier resigned from this revision.Wed, Jul 26, 6:11 AM
avt77 updated this revision to Diff 108268.Wed, Jul 26, 7:01 AM

I re-based the the code after the patch about replacing of assertions with corresponding diagnostic error messages. As result changes in tests show that X86 Asm supports symbolic Scale/Disp properly now (including its negative value).

avt77 added a comment.Thu, Aug 3, 2:39 AM

Hi Reviewers?
Do you have any questions about this patch?

RKSimon added inline comments.Thu, Aug 3, 10:20 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
571–572

(typo) ErrMsg = "Invalid operand of multiplication";

1461

clang-format

1485

clang-format

RKSimon added inline comments.Thu, Aug 3, 10:27 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1564

What is this accomplishing? Pulling on ImmVal makes sense, but the rest looks wrong - for instance, if Disp is a MCExpr::Constant doesn't that constant value is overriden by Imm instead of being added together?

avt77 added inline comments.Fri, Aug 4, 1:58 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1461

I'm using clang-format for every new patch prepared for phab and this style was generated by clang-format (I've just checked it again).

1485

See above.

1564

Oh, yes: if Disp is Constant and Imm != 0 then we should add 2 values and it was already done in one of the previous versions but disappeared. Tnank you. I'll add this clause asap. But the current override of Disp with Imm is correct as well because we have Disp == SM.getSym(). On the other hand the condition "if (!Disp || (Disp->getKind() == llvm::MCExpr::Constant && ImmVal))" looks wrong.

avt77 added inline comments.Fri, Aug 4, 3:13 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1564

Oh and Oh: I recalled the situation:

if (Disp is Constant) it means Disp == SM.getSym() (see assertion): it's the same value that's why we don't replace one with other. I could simplify it like here:

else if (!Disp)
  Disp = Imm; // It can be an immediate displacement only (maybe 0).

OK?
One more time: if (SM.getSym() != nullptr) and it's a constant then SM.getImm() returns the value of the constant.

avt77 updated this revision to Diff 109733.Fri, Aug 4, 7:29 AM

Hope, my comments now are OK.

A few minors - does anyone have any more comments?

lib/Target/X86/AsmParser/X86AsmParser.cpp
1554

Another use of SM.getImm() - is it safe to move SM.getImm() earlier to replace this as well?

1564

Flip to match the previous if():

else if (ImmVal && Disp->getKind() == llvm::MCExpr::Constant) {
test/MC/X86/intel-syntax-3.s
9–10

Now that these are passing - move them to another of the intel-syntax test files and add suitable checks

avt77 added inline comments.Tue, Aug 8, 12:50 AM
test/MC/X86/intel-syntax-3.s
9–10

In fact I kept it here specialy to show the difference but OK: I'll do it.

avt77 updated this revision to Diff 110154.Tue, Aug 8, 2:16 AM

The last requirements from Simon were fixed.

coby added a reviewer: coby.Tue, Aug 8, 2:56 AM
coby added a comment.Tue, Aug 8, 3:02 AM

A few minors - does anyone have any more comments?

Will have a look by tomorrow (at most)

coby added inline comments.Wed, Aug 9, 10:39 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
599

if comment (#) evaluate to false (i.e. MSVC inline-assembler does not support Assembly Constants, and we do not plan to do it either) than you should error here appropriately (i.e. "symbolic scale aren't supported on inline-asm)

601

BaseReg + SymScale * IndexReg

601

What about other operations?
If we are to deal with a symbolic constant as a constant than it should be able to participate in whatever operation an integer is allowed

604

(#)
Note that the two conditions (isParsingInlineAsm) and (SymRef->getKind() == llvm::MCExpr::Constant) never coexist in the current code due to the way Assembly Constants are being treated when parsing MS InlineAsm.
I'm not sure whether it is intended or not - does MSVC inline assembler allow any kind of Assembly Constants?

605

Note that this patch breaks 'ms-inline-asm.c' on 'clang/test/Sema'

995

Rebase
This is a part of an old code of mine which got reverted

1434–1440

I suggest we'll adopt a more general approach and deal with assembly constants as a whole, and not just this private case. wouldn't it simplify the work?

1549
if (Disp = SM.getSym() && isParsingInlineAsm())
coby added inline comments.Wed, Aug 9, 10:46 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1432

Suggested simplification (instead of the other changes as well):

} else if (!isParsingInlineAsm()) {
  if (getParser().parsePrimaryExpr(Val, End))
    return Error(Tok.getLoc(), "Unexpected identifier!");
  if (Val->getKind() == MCExpr::Constant) {
    int64_t TmpInt;
    Val->evaluateAsAbsolute(TmpInt);
    StringRef ErrMsg;
    if (SM.onInteger(TmpInt, ErrMsg))
      return Error(Tok.getLoc(), ErrMsg);
  } else
    SM.onIdentifierExpr(Val, Identifier);

Will allow the use of an assembly constant as an immediate, as integral part of whatever compound expression (scale / other).

coby added a reviewer: rnk.Wed, Aug 9, 10:47 AM

adding Reid.

avt77 added inline comments.Thu, Aug 10, 3:41 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
604

Do you mean that this function can't be called if isParsingInlineAsm == true?

605

I'm no sure I understand you here. As result of the patch we have the following:

void t3() {

  • __asm { mov eax, [eax] UndeclaredId } // expected-error {{unknown token in expression}} expected-error {{use of undeclared label 'UndeclaredId'}}

+ __asm { mov eax, [eax] UndeclaredId } // expected-error {{invalid address operation}} expected-error {{use of undeclared label 'UndeclaredId'}}

// FIXME: Only emit one diagnostic here.
// expected-error@+3 {{use of undeclared label 'A'}}
// expected-error@+2 {{unexpected type name 'A': expected expression}}
  • // expected-error@+1 {{unknown token in expression}}

+ // expected-error@+1 {{invalid address operation}}

__asm { mov eax, [eax] A }

}

As you see "unknown token" message was replaced by "invalid address operation". Does it mean "break" for this test?

avt77 added inline comments.Thu, Aug 10, 3:43 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
605

Don't know the reason but "bullet" above means "minus".

coby added inline comments.Thu, Aug 10, 3:51 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
604

I mean that (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) will never yield 'true' on current code state.
IMHO, we should aim for assembly level only.
So, combining the two we may limit this patch to Assembly parsing only

605

As long as the test remains unchanged :)
But again, I don't think that MS InlineAsm should be affected by this change.

avt77 added inline comments.Thu, Aug 10, 4:30 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
604

In this case I could simply produce error message if (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) , right?

coby added inline comments.Thu, Aug 10, 5:05 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
604

Currently, yes. But I frankly believe we should completely ignore InlineAsm (i.e. confine the patch to assembly level only, as proposed on line 1461), or conduct a better research regarding the way assembly constants are represented (likely - aren't) by msvc inline-assembler, and act upon the findings

avt77 updated this revision to Diff 110682.Fri, Aug 11, 3:23 AM

I applied all changes suggested by Coby.

coby added a comment.Sat, Aug 12, 11:41 AM

I applied all changes suggested by Coby.

I'm sorry, I guess I haven't been clear: I meant that the suggested code blob at line 1449 is a practical replacement of the entire patch (apart from the added diagnostics).
It seems to pass your added tests, and not to break anything else, Can you verify it answers your cause(s)?

avt77 updated this revision to Diff 110930.Mon, Aug 14, 2:52 AM

I removed all changes (apart from the added diagnostics). Now it should fit to Coby requirements. Right?

coby added a comment.Mon, Aug 14, 4:11 AM

few minors + add fixes in clang tests to adopt the added diagnostics.
nothing else has caught my eye

lib/Target/X86/AsmParser/X86AsmParser.cpp
593

You don't need this line

606

CurrState is redundant, you can use 'PrevState = State' as in the original code.

1438

line is redundant (ErrMsg is defined on the encapsulating function's scope)

avt77 updated this revision to Diff 111145.Tue, Aug 15, 5:09 AM

Last minor fixes done.

avt77 updated this revision to Diff 111146.Tue, Aug 15, 5:12 AM

The Clang changes added.

avt77 updated this revision to Diff 111154.Tue, Aug 15, 6:25 AM

I restored the patch with LLVM part only.

I created a ne review to show changes in Clang: D36735

rnk added inline comments.Tue, Aug 15, 9:44 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
579–591

Call this NewScale or something more informative than _Scale.

1437

evaluateAsAbsolute may fail. I think you should rewrite this like so:

if (const auto *CE = dyn_cast<MCConstantExpr>(Val)) {
  if (SM.onInteger(CE->getValue(), ErrMsg)
    return Error(Tok.getLoc(), ErrMsg);
}

That's all evaluateAsAbsolute is doing for you behind the scenes anyway.

avt77 updated this revision to Diff 111323.Wed, Aug 16, 3:17 AM

Aplied Reid's requirements.

Coby, Reid, is eveything OK? Could you get LGTM?

coby added a comment.Fri, Aug 18, 1:37 AM

Coby, Reid, is eveything OK? Could you get LGTM?

added one minor.
+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

lib/Target/X86/AsmParser/X86AsmParser.cpp
1434

this line is redundant, its semantics contained in the check performed on the next one.

avt77 added a comment.Fri, Aug 18, 3:26 AM

+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

Yes, I see that everything I need here is included in D36793. When are you going to commit it? BTW, I have some questions about D36793 but I'll include them in that review.

Finally, as I understand I should abandom this review because now it's a part of D36793 (including tests, etc.), right?

coby added a comment.Sun, Aug 20, 6:10 AM

+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

Yes, I see that everything I need here is included in D36793. When are you going to commit it?

as soon as it'll get approved..

Finally, as I understand I should abandom this review because now it's a part of D36793 (including tests, etc.), right?

indeed. note that your added diagnostics are not part of D36793, so consider adding them afterwards if you still fancy it.

avt77 abandoned this revision.Tue, Aug 22, 4:18 AM

All changes from this review (excluding error diagnostic) were included in D36793.