[llvm] 73442da - [X86][NFC] Remove useless code in X86MCCodeEmitter.cpp

[Philip Reames via llvm-commits]

I disagree that asserts on two specific cases because the switch isn't 
covered don't have much value.  I think it's highly dependent on the 
surrounding code structure (which admittedly, I don't have familiarity 
with), but it's not a generally true statement.

Your suggestion of having a single default with error on a covered 
switch makes a lot of sense to me.  If you want to add that, it would 
address my concern.

Philip

On 2/24/23 08:12, Kan, Shengchen wrote:
> No single instruction is a VEX/EVEX prefix, so I removed the PrefixByte format in emitVEXOpcodePrefix and let it fell into the llvm_unreachable in default label. I believe we agree on this.
>
> I think the questionable change to you is the removal of the two asserts in emitREXPrefix. More formats like MRMSrcMem4VOp3, MRMSrcMemOp4 etc are not supported by REX prefix too.
> The switch case in emitREXPrefix is not fully covered, so two asserts there doesn't have much value.
>
> And I think we should enumerate all the formats supported by REX prefix in emitREXPrefix and add a default label like emitVEXOpcodePrefix.
>
> Best
> Shengchen
>
> -----Original Message-----
> From: Philip Reames <listmail at philipreames.com>
> Sent: Friday, February 24, 2023 11:24 PM
> To: Kan, Shengchen <shengchen.kan at intel.com>; Shengchen Kan <llvmlistbot at llvm.org>; llvm-commits at lists.llvm.org
> Subject: Re: [llvm] 73442da - [X86][NFC] Remove useless code in X86MCCodeEmitter.cpp
>
> Since the main thing this change does is remove asserts, it seems rather questionable to me.  I will leave this to the target maintainers, but this looks likely worthy of revert unless there's a stronger justification than the commit message implies.
>
> Philip
>
> On 2/21/23 22:03, Shengchen Kan via llvm-commits wrote:
>> Author: Shengchen Kan
>> Date: 2023-02-22T14:02:47+08:00
>> New Revision: 73442daca11e0a6cbd0cfb7c765802eee8110527
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/73442daca11e0a6cbd0cfb7c76
>> 5802eee8110527
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/73442daca11e0a6cbd0cfb7c76
>> 5802eee8110527.diff
>>
>> LOG: [X86][NFC] Remove useless code in X86MCCodeEmitter.cpp
>>
>> Neither the switch in A nor the switch in B is fully covered, so we
>> don't need write an impossible format there.
>>
>> Added:
>>       
>>
>> Modified:
>>       llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>>
>> Removed:
>>       
>>
>>
>> ######################################################################
>> ########## diff  --git
>> a/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>> b/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>> index 5e9aaeff8b483..d48777b79242a 100644
>> --- a/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>> +++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>> @@ -949,7 +949,6 @@ PrefixKind X86MCCodeEmitter::emitVEXOpcodePrefix(int MemOperand,
>>      }
>>      case X86II::MRM_C0:
>>      case X86II::RawFrm:
>> -  case X86II::PrefixByte:
>>        break;
>>      case X86II::MRMDestMemFSIB:
>>      case X86II::MRMDestMem: {
>> @@ -1246,10 +1245,6 @@ PrefixKind X86MCCodeEmitter::emitREXPrefix(int MemOperand, const MCInst &MI,
>>      case X86II::MRM7r:
>>        Prefix.setB(MI, CurOp++);
>>        break;
>> -  case X86II::MRMr0:
>> -    llvm_unreachable("MRMr0 format never need REX prefix!");
>> -  case X86II::MRMDestMemFSIB:
>> -    llvm_unreachable("FSIB format never need REX prefix!");
>>      }
>>      PrefixKind Kind = Prefix.determineOptimalKind();
>>      if (Kind && UsesHighByteReg)
>>
>>
>>           
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits