Page MenuHomePhabricator

[WebAssembly] Fix FastISel address calculation bug
ClosedPublic

Authored by tlively on Aug 7 2020, 9:09 PM.

Details

Summary

Fixes PR47040, in which an assertion was improperly triggered during
FastISel's address computation. The issue was that an Address set to
be relative to the FrameIndex with offset zero was incorrectly
considered to have an unset base. When the left hand side of an add
set the Address to be 0 off the FrameIndex, the right side would not
detect that the Address base had already been set and could try to set
the Address to be relative to a register instead, triggering an
assertion.

This patch fixes the issue by explicitly tracking whether an Address
has been set rather than interpreting an offset of zero to mean the
Address has not been set.

Diff Detail

Event Timeline

tlively created this revision.Aug 7 2020, 9:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 9:09 PM
tlively requested review of this revision.Aug 7 2020, 9:09 PM
aheejin accepted this revision.Aug 8 2020, 3:09 AM
aheejin added inline comments.
llvm/test/CodeGen/WebAssembly/fast-isel-pr7040.ll
3 ↗(On Diff #284113)

The same typo for CL/commit message and the test file name

This revision is now accepted and ready to land.Aug 8 2020, 3:09 AM
sbc100 added a comment.Aug 8 2020, 8:29 AM

"The issue was that an Address set to
be relative to the FrameIndex with offset zero was incorrectly
considered to have an unset base. "

From the patch I can't see how offset (which I assume means the Offset member) comes into play at all. It looks like the fix applies to when either the FI or the Ref itself is zero. Or should I read FrameIndex with offset zero to mean FrameIndex of zero ?

tlively edited the summary of this revision. (Show Details)Aug 8 2020, 12:56 PM

"The issue was that an Address set to
be relative to the FrameIndex with offset zero was incorrectly
considered to have an unset base. "

From the patch I can't see how offset (which I assume means the Offset member) comes into play at all. It looks like the fix applies to when either the FI or the Ref itself is zero. Or should I read FrameIndex with offset zero to mean FrameIndex of zero ?

Oh you're right. It's not the Offset but the FrameIndex itself. I'll fix the commit message and comment.

This revision was landed with ongoing or failed builds.Aug 8 2020, 3:23 PM
This revision was automatically updated to reflect the committed changes.