[PATCH] D75203: Relax existing instructions to reduce the number of nops needed for alignment purposes

[Philip Reames via Phabricator via llvm-commits]

reames marked 7 inline comments as done.
reames added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:793
       dump(); });
 
+  optimizeLayout(Layout);
----------------
MaskRay wrote:
> I suspect we may have to do `while (layoutOnce(Layout))` and `optimizeLayout(Layout)` in a lockstep.
> 
> `optimizeLayout` may cause some JCC_1/JMP_1 in MCRelaxableFragment to need relaxation.
The whole point of the design is that we don't have to iterate them.  If you see a case where we do, please point it out; that's a bug.

See the comments about which starting offsets are changing and why we're not skipping over any relaxable fragment.  


================
Comment at: llvm/lib/MC/MCAssembler.cpp:794
 
+  optimizeLayout(Layout);
+
----------------
MaskRay wrote:
> We can dump the layout only when something has changed. This requires a change to `optimizeLayout`'s return type.
I see no value in this.  I can make the change if you want, but I don't think it's actually useful.  


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1198-1200
+      if (F.getKind() ==  MCFragment::FT_Data)
+        // Skip and ignore
+        continue;
----------------
MaskRay wrote:
> skan wrote:
> > `F.getKind() ==  MCFragment::FT_Data || F.getKind() == FT_CompactEncodedInst`
> > I think we can handle `MCCompactEncodedInstFragment` here.
> It seems we can use `switch (F.getKind())` here.
I'd written it with a switch originally.  It was much harder to read.  I could use a switch for the filtering and then fall into the complicated logic for the align cases if you want?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:656
+  MCInst Relaxed;
+  relaxInstruction(RF.getInst(), *RF.getSubtargetInfo(), Relaxed);
+
----------------
MaskRay wrote:
> `*RF.getSubtargetInfo()` -> `STI` (it is a member variable)
I'd prefer to leave this as is.  It follows the idiom of the other uses of relaxInstruction, and I haven't cross checked that the two subtarget infos are identical.  


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:31
+
+  # Check that we're not shifting aroudn the offsets of labels - doing
+  # that would require a further round of relaxation
----------------
skan wrote:
> around
No.  That would change meaning of comment.


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:6
+	.text
+	.section	.text
+  # Demonstrate that we can relax instructions to provide padding, not
----------------
MaskRay wrote:
> `.text` and `.section .text` do the same thing. We can actually delete `.file`, `.text` and `.section .text`.
We need to know it's a text section not a data section.


================
Comment at: llvm/test/MC/X86/align-via-relaxation.s:10
+  # CHECK: .text
+  # CHECK: 0: eb 1f                        	jmp 31 <foo>
+  # CHECK: 2: e9 1a 00 00 00               	jmp 26 <foo>
----------------
MaskRay wrote:
> `jmp` is misaligned.
Er, what?  Are you possibly thinking of the branch-align feature?  That's not enabled in this test file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75203/new/

https://reviews.llvm.org/D75203