This is an archive of the discontinued LLVM Phabricator instance.

Fix MachineInstr::findRegisterUseOperandIdx subreg checks
ClosedPublic

Authored by rampitec on Nov 5 2018, 4:57 PM.

Details

Summary

The function only checks that instruction reads a super-register
containing requested physical register. In case if a sub-register
if being read that is also a use of a super-reg, so added the check.
In particular MI->readsRegister() is broken because of the missing
check. The resulting check is essentially regsOverlap().

Diff Detail

Event Timeline

rampitec created this revision.Nov 5 2018, 4:57 PM
rampitec updated this revision to Diff 172709.Nov 5 2018, 9:03 PM
rampitec edited the summary of this revision. (Show Details)
rampitec removed reviewers: junbuml, sbaranga, sdardis.

I have realized the old check was also needed, an instruction reads a register in both cases: if the operand is a subreg of a requested register or if it is a superreg.

With the new patch previous changes to other targets are gone. Therefor gone the ask to review their test changes.

rampitec updated this revision to Diff 172711.Nov 5 2018, 9:57 PM
rampitec edited the summary of this revision. (Show Details)

The effective check is just regsOverlap().

arsenm added a subscriber: arsenm.Nov 6 2018, 11:25 AM
arsenm added inline comments.
lib/CodeGen/MachineInstr.cpp
938–939

Why is TRI optional here? I would expect it to be mandatory if there is a physical register and assert

rampitec added inline comments.Nov 6 2018, 12:11 PM
lib/CodeGen/MachineInstr.cpp
938–939

That's a good question why, but it is optional.
I have tried to use just TRI->regsOverlap() instead of the whole condition, and then run into crashes due to TRI being nullptr.

rampitec added inline comments.Nov 6 2018, 1:12 PM
lib/CodeGen/MachineInstr.cpp
938–939

Here are just some examples of places where the function is called without optional TRI operand and with a physreg:

lib/Target/AArch64/AArch64InstrInfo.cpp:

1515│ case AArch64::FCSELDrrr: {
1516├> int Idx = Instr.findRegisterUseOperandIdx(AArch64::NZCV);

lib/Target/AMDGPU/SIInstrInfo.cpp:

4769├> if (MI.findRegisterUseOperandIdx(AMDGPU::SCC) != -1)
4770│ Worklist.insert(&MI);

lib/Target/ARM/ARMLoadStoreOptimizer.cpp:

891├> bool BaseKill = LatestMI->killsRegister(Base);

and then:

include/llvm/CodeGen/MachineInstr.h:

1134│ bool killsRegister(unsigned Reg,
1135│ const TargetRegisterInfo *TRI = nullptr) const {
1136├> return findRegisterUseOperandIdx(Reg, true, TRI) != -1;

There is huge number of places across all targets.

rampitec added inline comments.Nov 6 2018, 1:40 PM
lib/CodeGen/MachineInstr.cpp
938–939

Altogether 2539 tests fail across all targets if TRI check is removed, even if physreg check is in place.

arsenm added inline comments.Nov 7 2018, 2:32 PM
lib/CodeGen/MachineInstr.cpp
938–939

Is there any real reason not to pass it in though? I would think it shouldn't be hard to make it mandatory, other than the manual work

rampitec added inline comments.Nov 7 2018, 2:41 PM
lib/CodeGen/MachineInstr.cpp
938–939

I do not think there is such a reason. That is just a lot of places across all backends.
Anyway, this is really orthogonal to this patch.

The code change looks fine to me, but it should be possible to cleanup the test case a bit.

test/CodeGen/AMDGPU/optimize-if-exec-masking.mir
135–151

It should be possible to remove most of the IR here.

760–781

It should be possible to remove most of this boilerplate.

rampitec updated this revision to Diff 173386.Nov 9 2018, 11:03 AM
rampitec marked 2 inline comments as done.

Test cleanup.

arsenm accepted this revision.Nov 12 2018, 9:31 AM

LGTM

This revision is now accepted and ready to land.Nov 12 2018, 9:31 AM
This revision was automatically updated to reflect the committed changes.