[PATCH] D78929: [AIX][XCOFF]emit extern linkage for the llvm intrinsic symbol

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:9-11
+; RUN: llvm-readobj --symbols %t.o | FileCheck --check-prefix=CHECKSYM %s
+; RUN: llvm-readobj --relocations --expand-relocs %t.o | FileCheck --check-prefix=CHECKRELOC %s
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > DiggerLin wrote:
> > > > jhenderson wrote:
> > > > > You can fold these two lines into one. No need for them to be separate:
> > > > > 
> > > > > ```
> > > > > ; RUN: llvm-readobj --symbols --relocations --expand-relocs %t.o | \
> > > > > ; RUN:   FileCheck --check-prefix=SYMRELOC %s
> > > > > ```
> > > > Thanks for your suggestion, but I think separating in two is based is more reasonable ,One test for test Symbol. other test for test relocation.  it is more readable.
> > > I'm not sure how it's more readable. It's very clear from my suggested command-line that we want to test the symbols and relocations (otherwise the options wouldn't be present), and would reduce the amount of work required to figure out which checks below correspond to which FileCheck line.
> > > 
> > > It would also be more in keeping with most tests I've worked with, and would reduce the number of executable invocations from 4 to 2, improving test run time.
> > I tried it with llvm-readobj --symbols --relocations --expand-relocs
> > it display the relocation information first and than  symbol information.
> > 
> > in the test case we want to verify symbol first and than relocation.
> > I have to keep current test.
> @hubert.reinterpretcast , what is your option on combine the
> ```  llvm-readobj --symbols ```
> and 
>  ```llvm-readobj --relocations --expand-relocs```
>  into 
> ```llvm-readobj --symbols --relocations --expand-relocs```
> 
> and than verify relocation information first as
> 
> ; SYMRELOC:    Relocation {{[{][[:space:]] *}}Virtual Address: 0x18
> ; SYMRELOC-NEXT:    Symbol: .memset (0)
> ; SYMRELOC-NEXT:    IsSigned: Yes
> ; SYMRELOC-NEXT:    FixupBitValue: 0
> ; SYMRELOC-NEXT:    Length: 26
> ; SYMRELOC-NEXT:    Type: R_RBR (0x1A)
> ; SYMRELOC-NEXT:  }
> 
> ; SYMRELOC:      Symbol {
> ; SYMRELOC-NEXT:     Index: 0
> ; SYMRELOC-NEXT:     Name: .memset
> ; SYMRELOC-NEXT:     Value (RelocatableAddress): 0x0
> ; SYMRELOC-NEXT:     Section: N_UNDEF
> ; SYMRELOC-NEXT:     Type: 0x0
> ; SYMRELOC-NEXT:     StorageCl
> ......
I suggest using `llvm-objdump -d --symbol-description` for the relocation check. As it is, we can't tell whether the relocation we found is in the right place. This happens to make it so that we end up with two separate tool invocations.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:32
+
+; CHECK:       .bar:
+; CHECK-NEXT: # %bb.0:                                # %entry
----------------
I believe I requested `CHECK-LABEL:` here.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:34
+; CHECK-NEXT: # %bb.0:                                # %entry
+; CHECK-NEXT:  mflr 0
+; CHECK:       bl .memset
----------------
Please indent the instructions in relation to the label.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:60
+
+; CHECKRELOC:    Relocation {{[{][[:space:]] *}}Virtual Address: 0x18
+; CHECKRELOC-NEXT:    Symbol: .memset (0)
----------------
Moot point, but this should be aligned to match the closing brace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929