[PATCH] D35319: LSE Atomics reorg - Part I
Tim Northover via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 13 07:17:43 PDT 2017
t.p.northover added a comment.
This diff covers lots of different areas:
1. DeadRegisterDefinitions: seems like a reasonable improvement,
2. The instruction definitions: horrible on the surface, but a massive bug might justify them. It's completely unclear why they're necessary (especially as this patch contains no tests).
3. The scheduling: whatever, assuming you have inside knowledge of ThunderX.
================
Comment at: lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp:141-142
+
+ for (unsigned I = 0; I < ND; ++I) {
+ const MachineOperand &MO = MI.getOperand(I);
+ if (!MO.isReg() || !MO.isDef())
----------------
This loop seems too generic: we know what instructions we're looking at, and we know that the entire purpose is to replace a register with XZR/WZR -- the surrounding code isn't going to do that if it's not in that class.
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:9501-9503
+let Predicates = [HasLSE], mayLoad = 1, mayStore = 1, hasSideEffects = 1 in
+class BaseSTOPEncoding<dag oops, dag iops, string asm, string operands,
+ string cstr, list<dag> pattern>
----------------
Why are you replacing an InstAlias with a real instruction? That's architecturally incorrect and only really done in LLVM when there's no other choice (weird encoding issues and so on in ARM).
================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:9581-9582
+
+class BaseNOPEncoding<dag oops, dag iops, string asm, string operands,
+ string cstr, list<dag> pattern>
+ : I<oops, iops, asm, operands, cstr, pattern> {
----------------
What does this have to do with atomics?
Repository:
rL LLVM
https://reviews.llvm.org/D35319
More information about the llvm-commits
mailing list