Don't try to legalize Intermediate instructions (with generic types)

[aditya_nandakumar]

Summary

During legalization, targets can create Pseudo Instructions with generic types. We shouldn't try to legalize them.

[qcolombet]

Hi Aditya,

It seems weird to me to have elements in the work list that we shouldn't process. Could we change the way we insert in the work list instead?
E.g., having the check you added within the lambda for recordInsertions.

Cheers,
-Quentin

[aditya_nandakumar]

Updated based on feedback - now we don't add to the list

[aditya_nandakumar]

Updated based on feedback.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

183

This line needs to happen unconditionally so that --debug-only=legalize reports the instruction.

[aditya_nandakumar]

lib/CodeGen/GlobalISel/Legalizer.cpp

183

Yes - for -debug-only printing output we would need it in the list but not process - but would disagree with Quentin's comment of not having the Instructions that are not needed in the work list.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

183

Incrementing NumNewInsns is just about how many instructions to print in the --debug-only=legalize output for that call to legalizeInstrStep(). The correlation with the growth of the work list is a coincidence.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

183

As you rightly pointed out, they are connected in implementation right now because it's printing from the work list iterators instead of the BB iterators. I've looked into refactoring this away and I believe I've fixed this locally. I'll post the patch on-list once I've tested it.

[dsanders]

lib/CodeGen/GlobalISel/Legalizer.cpp

183

The refactor is in D32744

[qcolombet]

LGTM

lib/CodeGen/GlobalISel/Legalizer.cpp

183

With @dsanders follow-on patch we don't need to push the NumNewInsns out of the if statement anymore. Thus, it sounds to me that all the concerns were solved.

[aditya_nandakumar]

Committed in r302199

lib/CodeGen/GlobalISel/Legalizer.cpp

for (auto MI = MBB.begin(); MI != MBB.end(); MI = NextMI) {
 
       // Only legalize pre-isel generic instructions: others don't have types
       // and are assumed to be legal.
       if (!isPreISelGenericOpcode(MI->getOpcode()))
         continue;
       unsigned NumNewInsns = 0;
       SmallVector<MachineInstr *, 4> WorkList;
       Helper.MIRBuilder.recordInsertions([&](MachineInstr *MI) {
+        // Only legalize pre-isel generic instructions.
+        // Legalization process could generate Target specific pseudo
+        // instructions with generic types. Don't record them
+        if (isPreISelGenericOpcode(MI->getOpcode())) {
           ++NumNewInsns;
           WorkList.push_back(MI);
+        }
       });
       WorkList.push_back(&*MI);
 
       bool Changed = false;
       LegalizerHelper::LegalizeResult Res;
       unsigned Idx = 0;
       do {
         Res = Helper.legalizeInstrStep(*WorkList[Idx]);