[PATCH] D59227: [GlobalISel][Utils] Teach getConstantVRegVal how to look through trunc and z|sext

[Matt Arsenault via llvm-commits]

> On Mar 13, 2019, at 3:59 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
> 
>> On Mar 13, 2019, at 11:08 AM, Matt Arsenault via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> 
>> arsenm added a comment.
>> 
>> In D59227#1427702 <https://reviews.llvm.org/D59227#1427702>, @qcolombet wrote:
>> 
>>> 
>> 
>> 
>> 
>> 
>>> I agree that the G_CONSTANT are a mov to a legal result register. However, I don't think they should be legalized on there own. Essentially, if we truncate/extend the constants directly during the legalization of their uses, they naturally become legal (the illegal ones become dead). I.e., we don't have to handle G_CONSTANT explicitly. This is true only if the builders don't create thing like ext(cst), like they do today, because then we indeed have to legalize those immediate moves.
>> 
>> I'm not sure why we would want special rules for constant handling.
> 
> Again that’s just my personal opinion, but my point is, because we don’t have to do anything with them when everything else do the right(TM) thing :).
> 
>> I think it would be cleaner to just keep them treated as any other normal instruction.
> 
> It’s because they are treated like normal instruction that we need this patch right now.
> 
>> I'm assuming you mean the legalizer by builder.
> 
> No I really meant builder. Admittedly we would require to do the constant folding for exts and truncs only in the legalizer, but at this point there isn’t much incentive to not be consistent with every pass.
> 

I’m not sure what you mean then. MachineIRBuilder just produces a constant with the given type. It doesn’t attempt to insert exts/truncates to “legalize” it.


>> If you're going to try to avoid the intermediate legalization artifacts by looking at the uses, I don't see why you wouldn't change the legalizer worklist to do this for every instruction.
> 
> That’s a fair point.
> I didn’t see it this way because ext(cst) are **not** simplified as part of legalization artifact, but produce simplifiable legalization artifacts after we legalize the constant ext(cst) => ext(truncates(cst)).
> That said, I am not in favor of any complicated scheme regarding how we handle the legalizer wordlist, but this makes me think that another way of looking at the problem is maybe we shouldn’t fail legalization before we are actually stuck.
> Let me illustrate on an example.
> 
> Let say we ask for the promotion of UREM for non-power-of-2 in:
> %cst = G_CONSTANT i14 cst
> = UREM i14 …, %cst
> 
> We will get:
> %cst = G_CONSTANT i14 cst
> %promotedCst = G_ZEXT i14 %cst to i16
> = UREM i16 …, %promotedCst
> 
> Then, we ask for lowering of UREM for i16:
> %cst = G_CONSTANT i14 cst
> %promotedCst = G_ZEXT i14 %cst to i16
> = UDIV i16 …, %promotedCst
> …
> 
> Now, the problem is we need to be able to legalize UDIV within this legalization step, I’ll come back to that, and let say we can only do that when RHS is a constant. So at this point, we have one of the following option:
> 1. Look through extensions (what this patch does)
> 2. Lower to a lib call (would need to be undone assuming the target supports this lib call at all)
> 3. Have promotedCst being an actual constant instead fo a zext of another one (which is not possible since we legalize constant and directly creating another constant may actually produce an illegal one and we’ll end up with an infinite loop)
> 
> Let say that we allow the legalization step to fail (unlike today) and requeue the instruction for later legalization. As long as we make progress elsewhere we keep going. Then the artifacts in the way of the constant would eventually be removed and we fixed the problem.
> 
> That actually sounds like a better solution and this is orthogonal to the way we handle constant.
> 
> What do you think?

It seems to me like this is more a problem with the artifacts than the constant itself. I have been somewhat confused about what the exact expectations are for the legalization artifacts (particularly for vectors), and feel like I’ve been hacking around them. There seems to be some hidden expectation that every truncate/extend combination is legal. If not, the legalizer immediately gives up on them. This leaves the function in a half legalized state, that would have been OK if the legalizer had continued. The artifact combiner would have eliminated the illegal ext/truncate pairs. If the legalizer continued as long as progress was made, this wouldn’t be an issue. I also haven’t entirely gotten comfortable with the idea of the artifact combiner being required for correctness, or verbalized some consistent rule for why it’s OK for a combine to be mandatory.

-Matt