This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][tablegen] Do not merge memoperands from instructions that weren't in the match.
ClosedPublic

Authored by dsanders on Jul 31 2017, 9:56 AM.

Details

Summary

Fix a bug discovered in an out-of-tree target where memoperands from
pseudo-instructions that weren't part of the match were being merged into the
result instructions as part of GIR_MergeMemOperands.

This bug was caused by a change to the handling of State.MIs between rules when
the state machine tables were fused into a single table. Previously, each rule
would reset State.MIs using State.MIs.resize(1) but this is no longer done, as a
result stale data is occasionally left in some elements of State.MIs. Most
opcodes aren't affected by this but GIR_MergeMemOperands merges all memoperands
from the intructions recorded in State.MIs into the result instruction.

Suppose for example, we processed but rejected the following pattern:

(signextend (load x))

at this point, State.MIs contains the signextend and the load. Now suppose we
process and accept this pattern:

(add x, y)

at this point, State.MIs contains the add as well as the (now irrelevant) load.
When GIR_MergeMemOperands is processed, the memoperands from that irrelevant
load will be merged into the result instruction even though it was not part of
the match.

Bringing back the State.MIs.resize(1) would fix the problem but it would limit
our ability to optimize the table in the future. Instead, this patch fixes the
problem by explicitly stating which instructions should be merged into the result.

There's no direct test case in this commit because a test case would be very brittle.
However, at the time of writing this should fix the failures in
http://green.lab.llvm.org/green/job/Compiler_Verifiers_GlobalISEL/ as well as a
failure in test/CodeGen/ARM/GlobalISel/arm-isel.ll when expensive checks are enabled.

Event Timeline

dsanders created this revision.Jul 31 2017, 9:56 AM
fhahn added a subscriber: fhahn.Aug 1 2017, 7:45 AM

It's been pointed out to me that one of the bots (http://green.lab.llvm.org/green/job/Compiler_Verifiers_GlobalISEL/) is red because of the bug this fixes. That being the case, I'm going to commit this and we can post-commit review it.

fhahn added a comment.Aug 2 2017, 2:33 AM

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
377

Does this mean that now we are advancing CurrentIdx till the end? Could this have an impact on further actions after GIR_MergeMemOperands?

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

Thanks. I'll commit it soon, I'm just re-running 'ninja check' at the moment.

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
377

Until the end of the variable-length MergeInsnID list which is indicated with a -1. After breaking out of this loop, CurrentIdx will be the index for the next GIR_* opcode.

fhahn added inline comments.Aug 2 2017, 2:51 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
377

Ah I thought it was something like that.

It might be slightly clearer if we would use a defined constant like END_VARARGS or something instead of -1, but that may be just because I have not much knowledge about GlobalISel

dsanders added inline comments.Aug 2 2017, 2:52 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
377

Ok, I'll make this change before the commit

dsanders edited the summary of this revision. (Show Details)Aug 2 2017, 4:02 AM
This revision was automatically updated to reflect the committed changes.