RFC: [Hexagon] Mark target as not "machine verifier clean"

[foad]

Summary

Machine verification fails after the HexagonPacketizer pass with various
errors like:

• Bad machine code: Using an undefined physical register ***
• Bad machine code: Non-terminator instruction after the first terminator ***

This seems to be the only reason why TargetPassConfig disables machine
verification for the passes immediately following addPreEmitPass. If we
mark Hexagon as not machine verifier clean, then we can enable machine
verification in more places for all the other targets that are clean.

[foad]

This is an RFC because I'm not sure it's really a good trade-off, to disable all verification of the Hexagon target just to get a tiny bit more verification on other targets. I'm open to other ideas. Is it feasible to fix the packetizer?

[MatzeB]

I wonder if hexagon could just do MachineRegisterInfo::invalidateLiveness to solve things, or if there's other issues.

Short of that, how about adding a bool FailsVerification; to the MachineFunction that target passes could set to skip further verification? That way hexagon could still keep verification enabled for earlier passes.

[MatzeB]

Another possibility would be for Hexagon to try doing their packetization/predication in addPreEmitPass2 which is actually immediately before emission rather than addPreEmitPass where people have added various passes afterwards...

[foad]

In D111386#3050227, @MatzeB wrote:

I wonder if hexagon could just do MachineRegisterInfo::invalidateLiveness to solve things, or if there's other issues.

Since writing this patch I have noticed that it's not just Hexagon, there are also problems in the AMDGPU target.

Short of that, how about adding a bool FailsVerification; to the MachineFunction that target passes could set to skip further verification? That way hexagon could still keep verification enabled for earlier passes.

How about D111397?

[kparzysz]

There is nothing to fix in the packetizer. On Hexagon the order of instructions inside of packets generally doesn't matter (with a few exceptions). The verifier simply doesn't handle that, and that's why it's disabled.

[kparzysz]

I guess you could add a target hook to TargetMachine to query whether the target wants to run the verifier at that stage, e.g.

addPass(createSomePreEmitPass(), TM->verifyCode(kPreEmit));

or something like that.

[MatzeB]

What about moving the packetizer pass from addPreEmitPass() to addPreEmitPass2()?

Currently we have the following running after PreEmitPass, but I have feeling none of that may be relevant for hexagon (and would likely fail if they were actually used with packetized code):

• FuncletLayout sorts exception handling funclets (I think funclets are only used on Windows and maybe Webassembly)
• StackMapLiveness computes information for stackmap instruction which are used by some languages with garbage collectors, unused in C/C++...
• Refines debug information; though I have a feeling that liveness analysis on a MachineFunction that doesn't pass the verifier isn't really producing sensible results...
• MachineOutliner, FunctionSplitter are those enabled/used by hexagon?

Anyway I think the gist is: If the HexagonPacketizer brings the MachineFunction into a form where it does not pass the verifier anymore then generic codegen passes likely won't do anything useful anymore either. But likely noone noticed because there are only some obscure passes running late. Anyway if that is true, then the easiest solution is to move the packetizer into addPreEmitPass2 so that it really is the last thing running before code emission.

[kparzysz]

That sounds reasonable. Let me try that locally (downstream). If it works, then let's do it.

[kparzysz]

One problem that came up is that the "BB Sections" pass runs before pre-emit2. This imposes restrictions on what kinds of optimizations we can run in pre-emit2. If we could move BB Sections until after pre-emit2, it would be a lot better for us.

[MatzeB]

One problem that came up is that the "BB Sections" pass runs before pre-emit2. This imposes restrictions on what kinds of optimizations we can run in pre-emit2. If we could move BB Sections until after pre-emit2, it would be a lot better for us.

I see, thanks for trying things out! The problem with moving "BB Sections" after addPreEmit2 is that again we are moving passes behind the supposedly last thing before emission. That is what happened to addPreEmit which prompted the addition of addPreEmit2 (and its silly name). For that reason I'd rather not start adding passes behind addPreEmit2 for now.

[foad]

In D111386#3050148, @foad wrote:

Is it feasible to fix the packetizer?

In D111386#3051208, @kparzysz wrote:

There is nothing to fix in the packetizer. On Hexagon the order of instructions inside of packets generally doesn't matter (with a few exceptions). The verifier simply doesn't handle that, and that's why it's disabled.

OK, maybe "fix" was the wrong word, but I still don't think it's good that the packetizer and the verifier disagree about the rules for valid MIR. Maybe the packetizer could set some MachineFunction property analogous to IsSSA, to tell the verifier that it should enforce different rules.

[kparzysz]

The problem is that the packet rules can be target-specific (there are some very Hexagon-specific situations that are allowed/disallowed). I'd rather have an option to turn off the verifier on packetized code, than to make significant efforts to make the verifier work with it. We do have our own verification code in MC that validates packets, so the MIR verifier adds little value for us.

[kparzysz]

I guess a flag DoNotVerify on the MF should be ok as well.

[kparzysz]

The limitation is really that we can't reorder basic blocks after the BB sections pass. I've been thinking about it some more and I think we could move the packetization into pre-emit2. There weren't any other issues uncovered when I tried it locally.

[MatzeB]

I think we can abandon this with D111397 landed?

[foad]

In D111386#3115822, @MatzeB wrote:

I think we can abandon this with D111397 landed?

Yes.

llvm/lib/CodeGen/TargetPassConfig.cpp

void TargetPassConfig::addMachinePasses() {
 
   addPreEmitPass();
 
   if (TM->Options.EnableIPRA)
     // Collect register usage information and produce a register mask of
     // clobbered registers, to be used to optimize call sites.
     addPass(createRegUsageInfoCollector());
 
-  // FIXME: Some backends are incompatible with running the verifier after
-  // addPreEmitPass.  Maybe only pass "false" here for those targets?
-  addPass(&FuncletLayoutID, false);
+  addPass(&FuncletLayoutID);
 
-  addPass(&StackMapLivenessID, false);
+  addPass(&StackMapLivenessID);
   addPass(&LiveDebugValuesID, false);
 
   if (TM->Options.EnableMachineOutliner && getOptLevel() != CodeGenOpt::None &&
       EnableMachineOutliner != RunOutliner::NeverOutline) {
     bool RunOnAllFunctions =
         (EnableMachineOutliner == RunOutliner::AlwaysOutline);
     bool AddOutliner =
         RunOnAllFunctions || TM->Options.SupportsDefaultOutlining;

llvm/lib/Target/Hexagon/HexagonTargetMachine.h

public:
   void adjustPassManager(PassManagerBuilder &PMB) override;
   void registerPassBuilderCallbacks(PassBuilder &PB) override;
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
   TargetTransformInfo getTargetTransformInfo(const Function &F) override;
 
   HexagonTargetObjectFile *getObjFileLowering() const override {
     return static_cast<HexagonTargetObjectFile*>(TLOF.get());
   }
+
+  bool isMachineVerifierClean() const override {
+    // Machine verification fails after HexagonPacketizer.
+    return false;
+  }
 };
 
 } // end namespace llvm
 
 #endif