[PATCH] D115419: [PowerPC] Allow absolute expressions in relocations

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:365-387
+  bool isS16ImmX4() const {
+    switch (Kind) {
+    case Expression:
+      return true;
+    case Immediate:
+    case ContextImmediate:
+      return isInt<16>(getImmS16Context()) && (getImmS16Context() & 3) == 0;
----------------
nickdesaulniers wrote:
> These two functions look very similar except for one constant. Consider calling a new helper that shares an implementation but accepts the constant as a parameter.
Sure, but I don't really think there's a way to pass parameters to it from the .td description (see lines 1018 and 1032 of PPCInstrInfo.td). I will turn them into wrappers for a function that does the computation.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.cpp:114
+    bool IsHalf16 =
+        Fixup && (unsigned)Fixup->getKind() == PPC::fixup_ppc_half16;
+    bool IsHalf16DS =
----------------
nickdesaulniers wrote:
> Using `Fixup->getTargetKind()` should avoid the need for the casts.
Sounds good.


================
Comment at: llvm/test/MC/PowerPC/ppc64-abs-reloc.s:3-7
+	.text
+	.abiversion 2
+	.file	"b.c"
+	.globl	test                            # -- Begin function test
+	.p2align	4
----------------
nickdesaulniers wrote:
> I'm guessing these directives and the cfi directives below are unnecessary to the functionality of this test? I'd be curious if this test case could be pared down further?
I just left what I got from the back end when compiling. I agree that they are not needed and will remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115419