[PATCH] D69411: [MC] Calculate difference of symbols in two fragments when possible

[Jian Cai via Phabricator via llvm-commits]

jcai19 added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:565
 
-  if ((&SecA != &SecB) && !Addrs)
+   if (SecB.getFragmentList().getNextNode(*FragB) == FragA &&
+       isa<MCDataFragment>(FragA) && isa<MCDataFragment>(FragB)) {
----------------
peter.smith wrote:
> I think it would be useful to add a comment explaining this line and what kind of expressions it is expected to support. It isn't intuitive given 
> how AttemptToFoldSymbolOffsetDifference is called.
> 
> 
This is inherited from the original code, except it was before the check for Layout originally. I am not exactly sure why this check is needed in the first place. In my opinion this is redundant with the check for Layout, as we can always calculate the differences of two symbols based on the order of the sections (MCAsmLayout::getSectionOrder ()) they are in and their offset within its own section respectively if the layout is provided., but I do need this check anyway for our case. Any thoughts on why this check is necessary?


================
Comment at: llvm/lib/MC/MCExpr.cpp:584
+    // the two symbols belong to two fragments in the same section.
+    // FIXME: can we resolve .if conditions while finalizing layout?
+    if (IsCond && SecB.getFragmentList().getNextNode(*FragB) == FragA &&
----------------
peter.smith wrote:
> It is difficult to see how it would be possible to resolve .if conditions at layout time in a single pass assembler. In theory the assembler could evaluate all conditional blocks and select between them at layout time, if such a layout could be converged on.
Thanks for the clarification, although I am not sure I follow.  The code looks iterative to me https://llvm.org/doxygen/MCAssembler_8cpp_source.html#l00785. I was thinking as the loop iterates and calls layoutOnce, we can relax (if needed) and calculate the sizes of the fragments before a .if statement and resolve the condition there. But I am not completely convinced by myself that it is doable. Also there some cases like the one below will create more complexity.

foo: jump to bar
...
.if . - foo = ${constant integer}
instr1
.else.
instr2
.endif
...
bar:

This creates a loop of dependency as depending on the instruction selected in the if-else block, the size of the jump instruction may change due to the number of bits it need to specify the offset, which in turn affects which instruction should be chosen.



================
Comment at: llvm/lib/MC/MCExpr.cpp:749
       if (Sym.getVariableValue()->evaluateAsRelocatableImpl(
-              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO)) {
+              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO, false)) {
         if (!IsMachO)
----------------
peter.smith wrote:
> peter.smith wrote:
> > /* IsCond */ false.
> We have IsCond passed in as a parameter to evaluateAsRelocatableImpl, so even if it is passed in true, we set it to false here? Is it important for the value to be false here? If so then it implies that IsCond might not be specific enough a name. If it just doesn't matter then can we pass in IsCond here?
Yes, thanks for the catching this. IsCond is never used here so I simply replaced it false. But passing in IsCond is a better choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411