<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 17 Nov 2017, at 10:15, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Nov 14, 2017, at 4:43 PM, Volkan Keles <<a href="mailto:vkeles@apple.com" class="">vkeles@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div class="">On Nov 13, 2017, at 4:32 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Nov 13, 2017, at 8:51 AM, Kristof Beyls via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""><br class="">kristof.beyls added inline comments.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:194-201<br class="">+ unsigned Op = MI.getOperand(i).getReg();<br class="">+ // G_UNMERGE_VALUES has variable number of operands, but there is only<br class="">+ // one source type and one destination type as all destinations must be the<br class="">+ // same type. So, get the last operand if TypeIdx == 1.<br class="">+ if (MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES && TypeIdx == 1) {<br class="">+ Op = MI.getOperand(MI.getNumOperands() - 1).getReg();<br class="">+ }<br class="">----------------<br class="">To make this easier to read, I think it may make sense to factor this out into a separate function, e.g. with a name like "getTyFromTypeIdx(MI, TypeIdx)". I'm not sure on the name, but doing it this way would make this code a lot easier to understand, I think.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:359<br class="">+ for (const auto &Ty :<br class="">+ {s32, s64, s96, s128, s192, v2s32, v3s32, v4s32, v2s64, v4s64, v6s64}) {<br class="">+ setAction({G_MERGE_VALUES, Ty}, Legal);<br class="">----------------<br class="">I haven't thought this through at all, but I'm a bit surprised v8s64 also isn't legal if v6s64 is?<br class="">Similarly for S256 if s192 is legal?<br class=""></blockquote><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">FWIW, I believe we should just specify the size (instead of full types) for operations that deal only with bit manipulation (merge_value, phi, bitwise operation, ld/st).</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"></div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="" style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69);">I think it’s good to legalize these operations based on full types because legalization actions might be different, especially for vector types.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Given that those instructions only deal with bag of bits, I don’t see why the full types are relevant.</div><div class="">What am I missing?</div></div></div></div></blockquote><div><br class=""></div><div>For some targets, an s64 load and an v2s32 load produce different bitpatterns in the register. For example, on big-endian Mips (and big-endian AArch64 I think) the elements are big-endian but element zero is still at offset 0 in memory and at bit 0 in the register:</div><div>Memory: 00 11 22 33 44 55 66 77</div><div>s64 load: 0x0011223344556677</div><div>v2s32 load: 0x4455667700112233</div><div><br class=""></div><div>Similarly, merge_values would need to behave differently depending for vectors and scalars:</div><div>s64 = G_MERGE_VALUES s32 0x00112233, s32 0x44556677 ; 0x0011223344556677</div><div>v2s32 = G_MERGE_VALUES s32 0x00112233, s32 0x44556677 ; 0x4455667700112233</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="" style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69);"> Otherwise, we’d need to mark it as Custom, check the full type and replace it with something else even if it’s legal.</div><div class="" style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69); min-height: 14px;"><br class=""></div><div class="" style="margin: 0px; font-stretch: normal; line-height: normal; font-family: "Helvetica Neue"; color: rgb(69, 69, 69);">Volkan</div></div><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""><br class=""><a href="https://reviews.llvm.org/D39823" class="">https://reviews.llvm.org/D39823</a></blockquote></div></blockquote></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></body></html>