[PATCH] D131347: Set up basic infrastructure for ELF/i386 backend support in JITLink .

[Kshitij Jain via Phabricator via llvm-commits]

jain98 marked 4 inline comments as done.
jain98 added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/i386.h:26
+  /// None
+  R_386_NONE = Edge::FirstRelocation,
+
----------------
sunho wrote:
> The enum name here is better to be non platform-specific like in x86_64.h and aarch64.h. The edges are going to be reused across other platforms too. (e.g. COFF/i386)
Makes sense. Changing it to `NONE`.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_i386.cpp:49
+
+  template <uint8_t Bits> static bool fitsRangeSignedInt(int64_t Value) {
+    return Value >= -(1 << Bits) && Value < (1 << Bits);
----------------
sunho wrote:
> If you were not aware of it, there is isInt<> function in support library.
This is actually a remnant from the starter code that I used from the introductory aarch64 jitlink [[ https://github.com/llvm/llvm-project/commit/2ed91da0f1f312aedcfa01786fe991e33c6cf1fe#diff-5189f3524394be80204b1d0a021a934c61e622dcbc47704e72602a7bce66ff4f | commit ]].

Removing the function entirely.


================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s:3
+# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t/i386_simple.o %s
+# RUN: llvm-jitlink %t/i386_simple.o; result=$?; [ $result -eq 42 ] || exit 1
+
----------------
lhames wrote:
> Tests should be `-noexec` (except in special circumstances): We want them to be able to run on architectures other than the target.
> 
> I would simplify to:
> ```
> # RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
> # RUN: llvm-jitlink -noexec %t.o
> ```
Does this also mean that the contents of this directory's `lit.local.cfg` should be changed? Currently this is what's in there - 

```
if not 'i386' in config.root.targets:
  config.unsupported = True
```

I may be wrong, but I //think// it's saying run this test only if the current host architecture is i386?



================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s:3
+# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t/i386_simple.o %s
+# RUN: llvm-jitlink %t/i386_simple.o; result=$?; [ $result -eq 42 ] || exit 1
+
----------------
jain98 wrote:
> lhames wrote:
> > Tests should be `-noexec` (except in special circumstances): We want them to be able to run on architectures other than the target.
> > 
> > I would simplify to:
> > ```
> > # RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
> > # RUN: llvm-jitlink -noexec %t.o
> > ```
> Does this also mean that the contents of this directory's `lit.local.cfg` should be changed? Currently this is what's in there - 
> 
> ```
> if not 'i386' in config.root.targets:
>   config.unsupported = True
> ```
> 
> I may be wrong, but I //think// it's saying run this test only if the current host architecture is i386?
> 
Additionally, what the reasoning behind making these tests `--noexec` - it looks like most JitLink tests are? I'm wondering what makes the code in these tests non-runnable if we're able to load it into memory? I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code. But then why go to the point where we're loading things into memory even? Why not stop after generating the `LinkGraph`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131347