This is an archive of the discontinued LLVM Phabricator instance.

[mips] Weaken asm predicate for memory offsets
ClosedPublic

Authored by sdardis on May 27 2016, 5:54 AM.

Details

Summary

The isMemWithSimmOffset predicate rejects relocations which is incorrect
behavior. Linkers and other tools should handle|warn|error when the
field overflows.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 58773.May 27 2016, 5:54 AM
sdardis retitled this revision from to [mips] Weaken asm predicate for memory offsets.
sdardis updated this object.
sdardis added reviewers: dsanders, vkalintiris.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
dsanders accepted this revision.May 27 2016, 6:38 AM
dsanders edited edge metadata.

LGTM with one nit about a comment.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1077 ↗(On Diff #58773)

I don't think we need this comment but if we do want to keep it then please put it in front of the 'template'

1080 ↗(On Diff #58773)

Hmm... This isn't quite correct but it's no less correct than the other predicates. I'm happy for this to be committed as-is but we need to fix this and a couple other predicates afterwards.

My concern is that checks for specific expression nodes will be confused by any variety. For example isConstantMemOff() is false for '1+1' because the top-level node is a MCBinaryExpr instead of an MCConstantExpr. Similarly, isMemWithSimmOffset() will unnecessarily reject things like '%lo(foo) + 2' when 'foo' is known. When 'foo' is unknown it will correctly reject it because %lo(foo)+2 is not a relocatable expression (it should be %lo(foo+2)).

I think we should do this kind of test with something along the lines of:

int64_t Offset;
if (getMemOff()->evaluateAsAbsolute(Offset) && isShiftedInt<Bits, ShiftAmount>(Offset))
  return true;
return getMemOff()->evaluateAsRelocatable(Value, ...);

Possibly with some checks that MCValue::RefKind agrees with the size of the field in the instruction

This revision is now accepted and ready to land.May 27 2016, 6:38 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review.