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

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

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

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.