[PATCH] D69411: [MC] Parse .if conditions with symbols in consecutive MCDataFragements

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

To summarise where I think we are right now.

- D76002 fixes a problem affecting within MCDataFragment eager evaluation of .if expressions. Specifically when there is a label that would be inserted into the MCDataFragment, but at the time of encountering the label the fragment hadn't been created. MC will attempt to reuse an MCDataFragment for new instructions, see CanReuseDataFragment() in MCObjectStreamer.cpp. As noted earlier when the Subtarget changes a new MCDataFragment is created. In the majority of cases this is done via a .arch or .arch_extension directive.
- This patch extends the eager evaluation to cope with two adjacent MCDataFragments. My understanding is that this only occurs in the following circumstances:
  - When bundle locking and --mc-relax-all is used, there is a complicated 1 instruction per fragment + fragment merging that goes on here. This is only used in NaCl which I'm not sure what the status of in Chrome is. I think it is at best deprecated.
  - When there is a Subtarget change between MCDataFragments.
- In all other cases such as .align, .space and a relaxable instruction there is a separate non MCDataFragment created so we cannot fix these up.
- The patch restricts the eager evaluation to .if as some asm backends do not want the expressions between fragments evaluated eagerly in some cases.

>From the perspective of the linux kernel, is D76002 sufficient? For example if the Subtarget changing directives are used in such a way that they don't create new MCDataFragments in a sensitive location then we may not need this. For example the following will assemble with D76002

          .arch_extension sec // Outside of .text
          
          .text
  9997:
          nop
  .if . - 9997b == 0
  // CHECK-NOT: error: expected absolute expression
  orr r1, r1, #1
  .else
  orr r1, r1, #2
  .endif

As will:

          .text
          .arch_extension sec                
          nop
  9997:
          nop
  .if . - 9997b == 0
  // CHECK-NOT: error: expected absolute expression
  orr r1, r1, #1
  .else
  orr r1, r1, #2
  .endif

Could the examples in the kernel be altered to not require this? It does seem like we are writing a lot of code for a small number of easily resolved cases.

I note that it is possible to write a contrived example that this patch can't handle (needs --triple=armv8a-linux-gnu as crypto and crc are V8 extensions) although I'm not suggesting implementing support for it.

          .text
  
          nop
  9997:
          .arch_extension crypto
          nop
          .arch_extension crc
          nop
  .if . - 9997b == 0
  // CHECK-NOT: error: expected absolute expression
  orr r1, r1, #1
  .else
  orr r1, r1, #2
  .endif        .text
  
          nop
  9997:
          .arch_extension crypto
          nop
          .arch_extension crc
          nop
  .if . - 9997b == 0
  // CHECK-NOT: error: expected absolute expression
  orr r1, r1, #1
  .else
  orr r1, r1, #2
  .endif

if.s:11:5: error: expected absolute expression
.if . - 9997b == 0



================
Comment at: llvm/lib/MC/MCExpr.cpp:587
+      MCFragment::FragmentType FragBTy = FragB->getKind();
+      // When there is no layout our ability to resolve differences between
+      // symbols is limited. In specific cases where the symbols are both
----------------
With the new information coming from D70062 I think we need to make this comment more specific. For example:

```
When the Subtarget is changed a new MCDataFragment is created. This handles the case of 
foo: instr; .arch_extension ext; instr .if . - foo
```



================
Comment at: llvm/test/MC/ARM/directive_if_offset.s:5
+nop
+.arch_extension sec
+9997:nop
----------------
Can we add a comment to explain the importance of .arch_extension, such as:
// Create a new MCDataFragment due to Subtarget change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411