[PATCH] D68341: [AIX] TOC pseudo expansion for 64bit large + 64bit small + 32bit large modes
Jason Liu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 8 10:57:34 PDT 2019
jasonliu added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:909
+
+ MCSymbol *MOSymbol = getMCSymbolForMO(MO, *this);
----------------
nit: MOSymbol could move down a bit to be closer to where it get used.
Also I would want to add const to this MOSymbol. But it seems that a lot of similar MOSymbols in this file do not have const with them, only add const for this one here would create inconsistency. Not sure if it's worth to have a NFC patch to add const for all the MOSymbol in this file first.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:3171
(PPCtoc_entry tglobaladdr:$disp, i32:$reg))]>;
-def ADDIStocHA : PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc_nor0:$reg, tocentry32:$disp),
- "#ADDIStocHA",
- [(set i32:$rD,
- (PPCtoc_entry i32:$reg, tglobaladdr:$disp))]>;
+let hasSideEffects = 0, isReMaterializable = 1 in {
+def ADDIStocHA: PPCEmitTimePseudo<(outs gprc:$rD), (ins gprc_nor0:$reg, tocentry32:$disp),
----------------
Curious about what the effect is for adding hasSideEffects and isReMaterializable.
We already have ADDIStocHA before for the other targets, but they did not require hasSideEffects = 0, and isReMaterializable = 1.
So what is special about the AIX target that we need to add them? Or is it simply an omission before and it's actually needed for the other targets as well?
I try to remove this line here and no test case would fail. If we need those, should we add test case for it?
================
Comment at: llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll:2
; RUN: llc -mtriple powerpc-ibm-aix-xcoff \
-; RUN: -code-model=small < %s | FileCheck %s
+; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL
+
----------------
Do we want to add -verify-machineinstrs to every llc invocation?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68341/new/
https://reviews.llvm.org/D68341
More information about the llvm-commits
mailing list