This is an archive of the discontinued LLVM Phabricator instance.

[GSel]: Add a cleanup combiner to cleanup legalization artifacts
ClosedPublic

Authored by aditya_nandakumar on Aug 18 2017, 10:02 AM.

Details

Summary

Added a combiner which can clean up truncs/extends that are created in order to make the types work during legalization.

Also moved the combineMerges to the LegalizeCombiner.

Looking forward to your feedback.

Diff Detail

Event Timeline

Added comment in the header section of LegalizeCombine.h

Previous update didn't have the comment section change staged and was missing from the diff I uploaded.

volkan added inline comments.Aug 25 2017, 3:23 AM
include/llvm/CodeGen/GlobalISel/LegalizerCombine.h
1 ↗(On Diff #111740)

This doesn't match with the filename. Is it LegalizerCombiner.h or LegalizerCombine.h?

2 ↗(On Diff #111740)

Could you remove the redundant dashes and format this line?

35 ↗(On Diff #111740)

MI.getOpcode() != TargetOpcode::G_ANYEXT would be better here.

38 ↗(On Diff #111740)
  • Missing check: DefMI && ....
  • Same here: DefMI->getOpcode() == TargetOpcode::G_TRUNC.

Since you check the same condition in the other functions too, I think it would be better if you could add a function for this.

39 ↗(On Diff #111740)

What about DEBUG(dbgs() << ".. Combine MI: " << *MI);?

51 ↗(On Diff #111740)

An empty line here would make the code easy to read.

52 ↗(On Diff #111740)

Same as above.

76 ↗(On Diff #111740)

An empty line here would make the code easy to read.

77 ↗(On Diff #111740)

Same as above.

107 ↗(On Diff #111740)

Same as above.

112 ↗(On Diff #111740)

Could you replace this with MRI.getVRegDef(SrcReg) and check if(MRI.getVRegDef(SrcReg))?

178 ↗(On Diff #111740)

An empty line here would make the code easy to read.

lib/CodeGen/GlobalISel/Legalizer.cpp
134

You do the combines after legalizing all of the recorded instructions, but a recorded instruction might be erased. So, I think we need to iterate over the MBB after the legalization.

igorb added a subscriber: igorb.Aug 28 2017, 10:00 AM
aditya_nandakumar marked 11 inline comments as done.Aug 28 2017, 2:39 PM

Thanks Volkan for your feedback. Replied/Updated to most comments.

include/llvm/CodeGen/GlobalISel/LegalizerCombine.h
35 ↗(On Diff #111740)

This comes from my habit of using the Constant on the left idiom (old habit so I don't accidentally do if (a =42) foo() which will silently compile where I meant to say ==, but with constant on the left, it won't accidentally compile). While I don't think one way is better than the other, I have updated as requested :).

38 ↗(On Diff #111740)

I didn't think the DefMI check here is necessary as it's impossible (illegal) to generate a G_[AZS]_EXT instruction where the input operand is not a register defined from an instruction. I can probably add it for completeness. Maybe an assert here is better?

112 ↗(On Diff #111740)

Is the check really necessary vs an assert?

lib/CodeGen/GlobalISel/Legalizer.cpp
134

I've added a check where if the legalization response is Legalized, we should assume it's erased and remove from the combine list. Is this what you had in mind? Is there some case where this check wouldn't be sufficient?

aditya_nandakumar marked 2 inline comments as done.

Updated based on feedback.

volkan edited edge metadata.Aug 29 2017, 10:42 AM

Thank you Aditya. I added a few more inline comments.

include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h
35

Could you clang-format this line?

56

Same as above.

59

Same as above.

83

Could you update this too? Just to make it consistent with the others.

lib/CodeGen/GlobalISel/Legalizer.cpp
128

DEBUG(dbgs() << ".. .. New MI: " << *WorkList[I]);

134

The problem is LegalizerHelper::Legalized doesn't mean all of the inserted instruction are legal. Targets can insert illegal instructions as they are going to be legalized anyway.

140

Same as above: dbgs() << ... << *DeadMI.

151

Same as above.

aditya_nandakumar marked 7 inline comments as done.Aug 29 2017, 11:58 AM
aditya_nandakumar added inline comments.
lib/CodeGen/GlobalISel/Legalizer.cpp
134

For each iteration of legalizeInstr, we get a return value Legalized (if legalized) and we assume that it's dead and remove from the combine list. When the additional instructions are legalized, depending on the legalizeAction, it will be Legalized (and removed from the CombineList) - or else combined.

Updated based on feedback.

volkan added inline comments.Aug 30 2017, 5:16 AM
lib/CodeGen/GlobalISel/Legalizer.cpp
134

I see now, thanks for explaining. That check should be enough then.

167

Do we still need this code block? I think the tryCombineInstruction call above already handles this case.

Updated based on Volkan's Feedback.

lib/CodeGen/GlobalISel/Legalizer.cpp
167

Yes - the tryCombine is currently only called on newly created instructions (legalization artifacts) where as we want to try to combine already existing merges.

Added a TODO comment about considering moving the Merges combining into a combiner pass

volkan accepted this revision.Aug 30 2017, 9:57 AM

Thank you Aditya, LGTM!

This revision is now accepted and ready to land.Aug 30 2017, 9:57 AM

Committed in r312158