[MC] Simplify the logic of applying fixup for fragments, NFCI

[skan]

Summary

Replace mutiple if else clauses with a switch clause and remove redundant checks. Before this patch, we need to add a statement like if(!isa<MCxxxFragment>(Frag)) here each time we add a new kind of MCEncodedFragment even if it has no fixups. After this patch, we don't need to do that.

[skan]

Update comments

[MaskRay]

Note that the number of lines actually increases... so I am not sure this is "simplification"

[skan]

In D83366#2138183, @MaskRay wrote:

Note that the number of lines actually increases... so I am not sure this is "simplification"

The increased line number comes from } and break, the logic is simplified and corrected since we do not need to check isa<MCEncodedFragment> here. We extract the code for "MCAlignFragment" out to make it clear that it doesn't have fixup. If you care
about the increased line number here, I can remove the unnecessary "{}" here or use if statement.

[MaskRay]

In D83366#2138199, @skan wrote:

In D83366#2138183, @MaskRay wrote:

Note that the number of lines actually increases... so I am not sure this is "simplification"

The increased line number comes from } and break, the logic is simplified and corrected since we do not need to check isa<MCEncodedFragment> here. We extract the code for "MCAlignFragment" out to make it clear that it doesn't have fixup. If you care
about the increased line number here, I can remove the unnecessary "{}" here or use if statement.

This is: if else if else if -> switch. I think it is just a refactoring and I agree that it is the right thing to do, but I don't find complexity being simplified.

llvm/lib/MC/MCAssembler.cpp

824

Can you move MCAlignFragment into the switch as well?

[skan]

In D83366#2138210, @MaskRay wrote:

This is: if else if else if -> switch. I think it is just a refactoring and I agree that it is the right thing to do, but I don't find complexity being simplified.

In addition to if else -> switch. The simplified thing is that we don't need to check isa<MCEncodedFragment>(&Frag) in the new code.
In the old code, if we want to add a new kind of fragment that inherits from MCEncodedFragment but is not a MCEncodedFragmentWithFixups, we have to add if(!isa<MCxxxFragment>(Frag) continue) here to avoid reaching "llvm_unreachable("Unknown fragment with fixups!")", it doesn't make sense since the new fragment has nothing to do with fixup.

[xiangzhangllvm]

That is really more clear than old code. I +1 for this refine.

[skan]

llvm/lib/MC/MCAssembler.cpp

824

ArrayRef<MCFixup> Fixups;
MutableArrayRef<char> Contents;
const MCSubtargetInfo *STI = nullptr;

for (const MCFixup &Fixup : Fixups) {
...

is not used by MCAlignFragment. I think an early continue here can make things more clear.

[MaskRay]

llvm/lib/MC/MCAssembler.cpp

824

They have trivial constructors. Moving it can be clearer/more efficient.

[skan]

Address review comments

[MaskRay]

Thanks!

[nickdesaulniers]

llvm/lib/MC/MCAssembler.cpp

836

if shouldInsertFixupForCodeAlign returns a bool, should we be checking the return code here?

[skan]

llvm/lib/MC/MCAssembler.cpp

836

I think not, the return value of shouldInsertFixupForCodeAlign depends on the target.

llvm/lib/MC/MCAssembler.cpp

void MCAssembler::layout(MCAsmLayout &Layout) {
 
   // Allow the object writer a chance to perform post-layout binding (for
   // example, to set the index fields in the symbol data).
   getWriter().executePostLayoutBinding(*this, Layout);
 
   // Evaluate and apply the fixups, generating relocation entries as necessary.
   for (MCSection &Sec : *this) {
     for (MCFragment &Frag : Sec) {
-      // Data and relaxable fragments both have fixups.  So only process
-      // those here.
-      // FIXME: Is there a better way to do this?  MCEncodedFragmentWithFixups
-      // being templated makes this tricky.
-      if (isa<MCEncodedFragment>(&Frag) &&
-          isa<MCCompactEncodedInstFragment>(&Frag))
-        continue;
-      if (!isa<MCEncodedFragment>(&Frag) && !isa<MCCVDefRangeFragment>(&Frag) &&
-          !isa<MCAlignFragment>(&Frag))
-        continue;
       ArrayRef<MCFixup> Fixups;
       MutableArrayRef<char> Contents;
       const MCSubtargetInfo *STI = nullptr;
-      if (auto *FragWithFixups = dyn_cast<MCDataFragment>(&Frag)) {
-        Fixups = FragWithFixups->getFixups();
-        Contents = FragWithFixups->getContents();
-        STI = FragWithFixups->getSubtargetInfo();
-        assert(!FragWithFixups->hasInstructions() || STI != nullptr);
-      } else if (auto *FragWithFixups = dyn_cast<MCRelaxableFragment>(&Frag)) {
-        Fixups = FragWithFixups->getFixups();
-        Contents = FragWithFixups->getContents();
-        STI = FragWithFixups->getSubtargetInfo();
-        assert(!FragWithFixups->hasInstructions() || STI != nullptr);
-      } else if (auto *FragWithFixups = dyn_cast<MCCVDefRangeFragment>(&Frag)) {
-        Fixups = FragWithFixups->getFixups();
-        Contents = FragWithFixups->getContents();
-      } else if (auto *FragWithFixups = dyn_cast<MCDwarfLineAddrFragment>(&Frag)) {
-        Fixups = FragWithFixups->getFixups();
-        Contents = FragWithFixups->getContents();
-      } else if (auto *AF = dyn_cast<MCAlignFragment>(&Frag)) {
+
+      // Process MCAlignFragment and MCEncodedFragmentWithFixups here.
+      switch (Frag.getKind()) {
+      default:
+        continue;
+      case MCFragment::FT_Align: {
+        MCAlignFragment &AF = cast<MCAlignFragment>(Frag);
         // Insert fixup type for code alignment if the target define
         // shouldInsertFixupForCodeAlign target hook.
-        if (Sec.UseCodeAlign() && AF->hasEmitNops()) {
-          getBackend().shouldInsertFixupForCodeAlign(*this, Layout, *AF);
-        }
+        if (Sec.UseCodeAlign() && AF.hasEmitNops())
+          getBackend().shouldInsertFixupForCodeAlign(*this, Layout, AF);
         continue;
-      } else if (auto *FragWithFixups =
-                     dyn_cast<MCDwarfCallFrameFragment>(&Frag)) {
-        Fixups = FragWithFixups->getFixups();
-        Contents = FragWithFixups->getContents();
-      } else
-        llvm_unreachable("Unknown fragment with fixups!");
+      }
+      case MCFragment::FT_Data: {
+        MCDataFragment &DF = cast<MCDataFragment>(Frag);
+        Fixups = DF.getFixups();
+        Contents = DF.getContents();
+        STI = DF.getSubtargetInfo();
+        assert(!DF.hasInstructions() || STI != nullptr);
+        break;
+      }
+      case MCFragment::FT_Relaxable: {
+        MCRelaxableFragment &RF = cast<MCRelaxableFragment>(Frag);
+        Fixups = RF.getFixups();
+        Contents = RF.getContents();
+        STI = RF.getSubtargetInfo();
+        assert(!RF.hasInstructions() || STI != nullptr);
+        break;
+      }
+      case MCFragment::FT_CVDefRange: {
+        MCCVDefRangeFragment &CF = cast<MCCVDefRangeFragment>(Frag);
+        Fixups = CF.getFixups();
+        Contents = CF.getContents();
+        break;
+      }
+      case MCFragment::FT_Dwarf: {
+        MCDwarfLineAddrFragment &DF = cast<MCDwarfLineAddrFragment>(Frag);
+        Fixups = DF.getFixups();
+        Contents = DF.getContents();
+        break;
+      }
+      case MCFragment::FT_DwarfFrame: {
+        MCDwarfCallFrameFragment &DF = cast<MCDwarfCallFrameFragment>(Frag);
+        Fixups = DF.getFixups();
+        Contents = DF.getContents();
+        break;
+      }
+      }
       for (const MCFixup &Fixup : Fixups) {
         uint64_t FixedValue;
         bool IsResolved;
         MCValue Target;
         std::tie(Target, FixedValue, IsResolved) =
             handleFixup(Layout, Frag, Fixup);
         getBackend().applyFixup(*this, Fixup, Target, Contents, FixedValue,
                                 IsResolved, STI);