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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the summary.

In D69411#1742030 <https://reviews.llvm.org/D69411#1742030>, @peter.smith wrote:

> 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.


I'll follow up on this.  There's likely a lot of code that can be dropped if that's the case.

>   - 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?

I can still reproduce https://github.com/ClangBuiltLinux/linux/issues/742 with https://reviews.llvm.org/D70062 applied.

> 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.

The case from the kernel is testing that a single instruction is 2B rather than 4B (narrow vs wide).  See https://github.com/ClangBuiltLinux/linux/blob/de620fb99ef2bd52b2c5bc52656e89dcfc0e223a/arch/arm/include/asm/assembler.h#L255-L270.

I don't see any subarch directives there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69411