This is an archive of the discontinued LLVM Phabricator instance.

[mips][ias] fix li macro when values are negated with ~
ClosedPublic

Authored by sdardis on Aug 11 2016, 3:27 AM.

Details

Summary

The integrated assembler evaluates the expressions such as ~0x80000000 to
0xffffffff7fffffff early in the parsing process. This patch adds compatibility
with gas so that li loads the expected value (0x7fffffff) in those cases. This
only occurs iff all the upper 32bits are set and maintains existing checks by not
truncating the result down to 32 bits if any of the the upper bits are not set.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 67668.Aug 11 2016, 3:27 AM
sdardis retitled this revision from to [mips][ias] extend li macro when values are negated with ~ .
sdardis updated this object.
sdardis added a reviewer: dsanders.
sdardis added a subscriber: llvm-commits.
dsanders edited edge metadata.Aug 19 2016, 4:01 AM

After discussing it with you off-list and experimenting with our gcc toolchain, I think you're on the right track. We need to accept the immediate if it is a simm33 and render it as a uimm32. This isn't quite the same as the 'relaxed' predicates so we should rename things as appropriate. This is slightly odd since li technically produces a signed value on the target but this doesn't affect the target behaviour.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1025–1035

This seems to be nearly equivalent to addConstantUImmOperands<32>(). The difference is this only truncates if the value is within 32-bit whereas addConstantUImmOperands<32> always truncates. I'd expect out-of-range numbers to have been rejected by the predicate. I think we should use addConstantUImmOperands<32> instead.

1100

This predicate is wrong and I think it's a key reason (but not the only reason) you needed to add a new predicate and renderer. It currently evaluates to false for expressions like MCUnaryExpr('~', MCConstantExpr(0x7fffffff)) and can only be true for MCConstantExpr by itself. It should be calling evaluateAsAbsolute() and returning true if that's successful. getConstantImm() will need updating to match.

lib/Target/Mips/MipsInstrInfo.td
454

This should probably take the immediate size for consistency (like SImmAsmOperandClass)

458

I think this should be isSImm<33>

489

This isn't needed, we can inherit it from UImmAnyAsmOperandClass

510–513

I've just noticed that one of these should probably be have UImm16AsmOperandClass as the superclass instead of UImm16RelaxedAsmOperandClass.

sdardis updated this revision to Diff 69220.Aug 25 2016, 3:42 AM
sdardis retitled this revision from [mips][ias] extend li macro when values are negated with ~ to [mips][ias] fix li macro when values are negated with ~ .
sdardis edited edge metadata.

Address reviewer comments.

Can you take a look at this Zoran? Thanks.

zoran.jovanovic accepted this revision.Sep 20 2016, 6:04 AM
zoran.jovanovic edited edge metadata.

LGTM.

lib/Target/Mips/MipsInstrInfo.td
2384

Nit: Is this comment still valid?

This revision is now accepted and ready to land.Sep 20 2016, 6:04 AM
sdardis marked an inline comment as done.Sep 20 2016, 7:34 AM
sdardis added inline comments.
lib/Target/Mips/MipsInstrInfo.td
2384

No, it's not. I'll change it when I commit to "We use uimm32_coerced to accept a 33 bit signed number that is rendered into a 32 bit number." or similar.

Looks like patch was not committed.

sdardis marked an inline comment as done.Oct 5 2016, 12:13 PM

Thanks, committed as r283353.

sdardis closed this revision.Oct 5 2016, 12:24 PM